Buildroot Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [RFC] fs: allow passing the image file name to inner-rootfs
@ 2018-06-22  2:31 Carlos Santos
  2018-10-21 13:26 ` Yann E. MORIN
  0 siblings, 1 reply; 15+ messages in thread
From: Carlos Santos @ 2018-06-22  2:31 UTC (permalink / raw)
  To: buildroot

Suppose that you want to create a filesystem image containing only the
/var/lib subtree. That would something like this on a br2-external:

fs/rootfs-var-lib/Config.in:

    menuconfig BR2_TARGET_ROOTFS_VAR_LIB
    bool "/var/lib"

    if BR2_TARGET_ROOTFS_VAR_LIB
    source "$BR2_EXTERNAL_MFS_PATH/fs/rootfs-var-lib/ext4/Config.in"
    endif

fs/rootfs-var-lib/rootfs-var-lib.mk:
    ifeq ($(BR2_TARGET_ROOTFS_VAR_LIB),y)
    rootfs-var-lib = $(call inner-rootfs,var-lib-$(pkgname),VAR_LIB_$(call UPPERCASE,$(pkgname)))
    include $(sort $(wildcard $(BR2_EXTERNAL_MFS_PATH)/fs/rootfs-var-lib/*/*.mk))
    endif

fs/rootfs-var-lib/ext4/Config.in:
    config BR2_TARGET_ROOTFS_VAR_LIB_EXT4
        bool "ext4 /var/lib filesystem"
        select BR2_PACKAGE_HOST_E2FSPROGS
        help
          Build an ext4 /var/lib filesystem

    if BR2_TARGET_ROOTFS_VAR_LIB_EXT4
        [...]
    endif # BR2_TARGET_ROOTFS_VAR_LIB_EXT4

fs/rootfs-var-lib/ext4/ext4.mk:
    # Build the ext4 /var/lib filesystem image
    [...]
    ROOTFS_VAR_LIB_EXT4_OPTS = \
        -d $(TARGET_DIR)/var/lib \
                        ^^^^^^^^---- /var/lib, only
    [...]
    define ROOTFS_VAR_LIB_EXT4_CMD
        rm -f $@
        $(HOST_DIR)/sbin/mkfs.ext4 $(ROOTFS_VAR_LIB_EXT4_OPTS) $@ "$(ROOTFS_VAR_LIB_EXT4_SIZE)"
    endef
    $(eval $(rootfs-var-lib))

Selecting BR2_TARGET_ROOTFS_VAR_LIB_EXT4 would produce the filesystem
image $(BINARIES_DIR)/rootfs.var-lib-ext4 coresponging to the contents
of $(TARGET_DIR)/var/lib. This is correct but the file name is a bit
awkward. It would be better to call it "rootfs-var-lib.ext4", instead.

This can be achieved by passing a third argument to inner-rootfs with
the desired file name:

    rootfs-var-lib = $(call inner-rootfs,var-lib-$(pkgname),VAR_LIB_$(call UPPERCASE,$(pkgname)),rootfs-var-lib.$(pkgname))

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
 fs/common.mk | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/common.mk b/fs/common.mk
index abf35418cb..2380c1dd3f 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -139,10 +139,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)/$(3): ROOTFS=$(2)
+$$(BINARIES_DIR)/$(3): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
+$$(BINARIES_DIR)/$(3): $$(ROOTFS_$(2)_DEPENDENCIES)
+	@$$(call MESSAGE,"Generating root filesystem image $(3)")
 	rm -rf $$(ROOTFS_$(2)_DIR)
 	mkdir -p $$(ROOTFS_$(2)_DIR)
 	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
@@ -163,7 +163,7 @@ endif
 rootfs-$(1)-show-depends:
 	@echo $$(ROOTFS_$(2)_DEPENDENCIES)
 
-rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1)
+rootfs-$(1): $$(BINARIES_DIR)/$(3)
 
 .PHONY: rootfs-$(1) rootfs-$(1)-show-depends
 
@@ -181,6 +181,6 @@ endif
 endef
 
 # $(pkgname) also works well to return the filesystem name
-rootfs = $(call inner-rootfs,$(pkgname),$(call UPPERCASE,$(pkgname)))
+rootfs = $(call inner-rootfs,$(pkgname),$(call UPPERCASE,$(pkgname)),rootfs.$(pkgname))
 
 include $(sort $(wildcard fs/*/*.mk))
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Buildroot] [RFC] fs: allow passing the image file name to inner-rootfs
  2018-06-22  2:31 [Buildroot] [RFC] fs: allow passing the image file name to inner-rootfs Carlos Santos
@ 2018-10-21 13:26 ` Yann E. MORIN
  2018-10-24  1:01   ` [Buildroot] [PATCH] fs: allow filesystems to set the name of their output file Carlos Santos
  2018-10-24  1:04   ` [Buildroot] [RFC] fs: allow passing the image file name to inner-rootfs Carlos Santos
  0 siblings, 2 replies; 15+ messages in thread
From: Yann E. MORIN @ 2018-10-21 13:26 UTC (permalink / raw)
  To: buildroot

Carlos, All,

On 2018-06-21 23:31 -0300, Carlos Santos spake thusly:
> Suppose that you want to create a filesystem image containing only the
> /var/lib subtree. That would something like this on a br2-external:
[--SNIP--]
> This can be achieved by passing a third argument to inner-rootfs with
> the desired file name:
>     rootfs-var-lib = $(call inner-rootfs,var-lib-$(pkgname),VAR_LIB_$(call UPPERCASE,$(pkgname)),rootfs-var-lib.$(pkgname))

We've discussed this patch during the developers days, and we came to
the conclusion that this should be better done by allowing filesystems
to provide the name of their output file as a variable, something like:

    ROOTFS_FOOBAR_IMAGE_NAME = my-own-filesystem.data

Then the infrastructure would do:

    ROOTFS_$(2)_IMAGE_NAME ?= rootfs.$(1)

    $$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME): ROOTFS=$(2)
    $$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
    [...and so on...]

Using a variable is more inline with how we handle this kind of
customisations in Buildrooot, and allows one to continue calling the
'rootfs' macro instad of the 'rootfs-inner' one (which, as its name
implies, is an inner macro, aka purely for internal use, not public).

Besides, the commit log was seen as too decorelated from the actual
change. It is not needed to explain the 'sub-tree-only' case, and just
state that some filesystems may want to tweak their output name, rather
than the current scheme.

Regards,
Yann E. MORIN.

> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
> ---
>  fs/common.mk | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/common.mk b/fs/common.mk
> index abf35418cb..2380c1dd3f 100644
> --- a/fs/common.mk
> +++ b/fs/common.mk
> @@ -139,10 +139,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)/$(3): ROOTFS=$(2)
> +$$(BINARIES_DIR)/$(3): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
> +$$(BINARIES_DIR)/$(3): $$(ROOTFS_$(2)_DEPENDENCIES)
> +	@$$(call MESSAGE,"Generating root filesystem image $(3)")
>  	rm -rf $$(ROOTFS_$(2)_DIR)
>  	mkdir -p $$(ROOTFS_$(2)_DIR)
>  	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
> @@ -163,7 +163,7 @@ endif
>  rootfs-$(1)-show-depends:
>  	@echo $$(ROOTFS_$(2)_DEPENDENCIES)
>  
> -rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1)
> +rootfs-$(1): $$(BINARIES_DIR)/$(3)
>  
>  .PHONY: rootfs-$(1) rootfs-$(1)-show-depends
>  
> @@ -181,6 +181,6 @@ endif
>  endef
>  
>  # $(pkgname) also works well to return the filesystem name
> -rootfs = $(call inner-rootfs,$(pkgname),$(call UPPERCASE,$(pkgname)))
> +rootfs = $(call inner-rootfs,$(pkgname),$(call UPPERCASE,$(pkgname)),rootfs.$(pkgname))
>  
>  include $(sort $(wildcard fs/*/*.mk))
> -- 
> 2.17.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH] fs: allow filesystems to set the name of their output file
  2018-10-21 13:26 ` Yann E. MORIN
@ 2018-10-24  1:01   ` Carlos Santos
  2018-10-24 14:36     ` Yann E. MORIN
  2018-10-25  0:23     ` Carlos Santos
  2018-10-24  1:04   ` [Buildroot] [RFC] fs: allow passing the image file name to inner-rootfs Carlos Santos
  1 sibling, 2 replies; 15+ messages in thread
From: Carlos Santos @ 2018-10-24  1:01 UTC (permalink / raw)
  To: buildroot

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)
+$$(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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Buildroot] [RFC] fs: allow passing the image file name to inner-rootfs
  2018-10-21 13:26 ` Yann E. MORIN
  2018-10-24  1:01   ` [Buildroot] [PATCH] fs: allow filesystems to set the name of their output file Carlos Santos
@ 2018-10-24  1:04   ` Carlos Santos
  1 sibling, 0 replies; 15+ messages in thread
From: Carlos Santos @ 2018-10-24  1:04 UTC (permalink / raw)
  To: buildroot

Superseded by https://patchwork.ozlabs.org/patch/988446/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH] fs: allow filesystems to set the name of their output file
  2018-10-24  1:01   ` [Buildroot] [PATCH] fs: allow filesystems to set the name of their output file Carlos Santos
@ 2018-10-24 14:36     ` Yann E. MORIN
  2018-10-25  0:20       ` [Buildroot] [PATCH v2] " Carlos Santos
  2018-11-03 10:37       ` [Buildroot] [PATCH] " Arnout Vandecappelle
  2018-10-25  0:23     ` Carlos Santos
  1 sibling, 2 replies; 15+ messages in thread
From: Yann E. MORIN @ 2018-10-24 14:36 UTC (permalink / raw)
  To: buildroot

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
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH v2] fs: allow filesystems to set the name of their output file
  2018-10-24 14:36     ` Yann E. MORIN
@ 2018-10-25  0:20       ` Carlos Santos
  2018-11-01 21:01         ` Yann E. MORIN
  2018-11-03 22:13         ` Carlos Santos
  2018-11-03 10:37       ` [Buildroot] [PATCH] " Arnout Vandecappelle
  1 sibling, 2 replies; 15+ messages in thread
From: Carlos Santos @ 2018-10-25  0:20 UTC (permalink / raw)
  To: buildroot

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.

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
Supersedes: https://patchwork.ozlabs.org/patch/988446/

Changes v1->v2:
- Prevent leading space when the filesystem gets 'imaginative', as
  pointed by Yann Morin.

NOTE: https://patchwork.ozlabs.org/patch/927116/ must be upated to
document this.
---
 fs/common.mk | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/common.mk b/fs/common.mk
index 453da6010a..9c2f9c6dd4 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -106,6 +106,8 @@ rootfs-common-show-depends:
 # all variable references except the arguments must be $$-quoted.
 define inner-rootfs
 
+ROOTFS_$(2)_IMAGE_NAME ?= rootfs.$(1)
+ROOTFS_$(2)_FINAL_IMAGE_NAME = $$(call qstrip,$$(ROOTFS_$()_IMAGE_NAME))
 ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1)
 ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target
 
@@ -140,10 +142,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)_FINAL_IMAGE_NAME): ROOTFS=$(2)
+$$(BINARIES_DIR)/$$(ROOTFS_$(2)_FINAL_IMAGE_NAME): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
+$$(BINARIES_DIR)/$$(ROOTFS_$(2)_FINAL_IMAGE_NAME): $$(ROOTFS_$(2)_DEPENDENCIES)
+	@$$(call MESSAGE,"Generating filesystem image $$(ROOTFS_$(2)_FINAL_IMAGE_NAME)")
 	rm -rf $$(ROOTFS_$(2)_DIR)
 	mkdir -p $$(ROOTFS_$(2)_DIR)
 	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
@@ -164,7 +166,7 @@ endif
 rootfs-$(1)-show-depends:
 	@echo $$(ROOTFS_$(2)_DEPENDENCIES)
 
-rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1)
+rootfs-$(1): $$(BINARIES_DIR)/$$(ROOTFS_$(2)_FINAL_IMAGE_NAME)
 
 .PHONY: rootfs-$(1) rootfs-$(1)-show-depends
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH] fs: allow filesystems to set the name of their output file
  2018-10-24  1:01   ` [Buildroot] [PATCH] fs: allow filesystems to set the name of their output file Carlos Santos
  2018-10-24 14:36     ` Yann E. MORIN
@ 2018-10-25  0:23     ` Carlos Santos
  1 sibling, 0 replies; 15+ messages in thread
From: Carlos Santos @ 2018-10-25  0:23 UTC (permalink / raw)
  To: buildroot

Superseded by https://patchwork.ozlabs.org/patch/988843/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH v2] fs: allow filesystems to set the name of their output file
  2018-10-25  0:20       ` [Buildroot] [PATCH v2] " Carlos Santos
@ 2018-11-01 21:01         ` Yann E. MORIN
  2018-11-03  2:20           ` Carlos Santos
  2018-11-03 22:13         ` Carlos Santos
  1 sibling, 1 reply; 15+ messages in thread
From: Yann E. MORIN @ 2018-11-01 21:01 UTC (permalink / raw)
  To: buildroot

Carlos, All,

On 2018-10-24 21:20 -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.
> 
> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
> ---
> Supersedes: https://patchwork.ozlabs.org/patch/988446/
> 
> Changes v1->v2:
> - Prevent leading space when the filesystem gets 'imaginative', as
>   pointed by Yann Morin.
> 
> NOTE: https://patchwork.ozlabs.org/patch/927116/ must be upated to
> document this.
> ---
>  fs/common.mk | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/common.mk b/fs/common.mk
> index 453da6010a..9c2f9c6dd4 100644
> --- a/fs/common.mk
> +++ b/fs/common.mk
> @@ -106,6 +106,8 @@ rootfs-common-show-depends:
>  # all variable references except the arguments must be $$-quoted.
>  define inner-rootfs
>  
> +ROOTFS_$(2)_IMAGE_NAME ?= rootfs.$(1)
> +ROOTFS_$(2)_FINAL_IMAGE_NAME = $$(call qstrip,$$(ROOTFS_$()_IMAGE_NAME))
                                                           ^^^

This patch has not been tested at all, it can't even possibly work...

Regards,
Yann E. MORIN.

>  ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1)
>  ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target
>  
> @@ -140,10 +142,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)_FINAL_IMAGE_NAME): ROOTFS=$(2)
> +$$(BINARIES_DIR)/$$(ROOTFS_$(2)_FINAL_IMAGE_NAME): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
> +$$(BINARIES_DIR)/$$(ROOTFS_$(2)_FINAL_IMAGE_NAME): $$(ROOTFS_$(2)_DEPENDENCIES)
> +	@$$(call MESSAGE,"Generating filesystem image $$(ROOTFS_$(2)_FINAL_IMAGE_NAME)")
>  	rm -rf $$(ROOTFS_$(2)_DIR)
>  	mkdir -p $$(ROOTFS_$(2)_DIR)
>  	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
> @@ -164,7 +166,7 @@ endif
>  rootfs-$(1)-show-depends:
>  	@echo $$(ROOTFS_$(2)_DEPENDENCIES)
>  
> -rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1)
> +rootfs-$(1): $$(BINARIES_DIR)/$$(ROOTFS_$(2)_FINAL_IMAGE_NAME)
>  
>  .PHONY: rootfs-$(1) rootfs-$(1)-show-depends
>  
> -- 
> 2.17.1
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH v2] fs: allow filesystems to set the name of their output file
  2018-11-01 21:01         ` Yann E. MORIN
@ 2018-11-03  2:20           ` Carlos Santos
  2018-11-03 22:25             ` Carlos Santos
  0 siblings, 1 reply; 15+ messages in thread
From: Carlos Santos @ 2018-11-03  2:20 UTC (permalink / raw)
  To: buildroot

> From: "Yann Morin" <yann.morin.1998@free.fr>
> To: "DATACOM" <casantos@datacom.com.br>
> Cc: "buildroot" <buildroot@buildroot.org>
> Sent: Quinta-feira, 1 de novembro de 2018 18:01:03
> Subject: Re: [PATCH v2] fs: allow filesystems to set the name of their output file

> Carlos, All,
> 
> On 2018-10-24 21:20 -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.
>> 
>> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
>> ---
>> Supersedes: https://patchwork.ozlabs.org/patch/988446/
>> 
>> Changes v1->v2:
>> - Prevent leading space when the filesystem gets 'imaginative', as
>>   pointed by Yann Morin.
>> 
>> NOTE: https://patchwork.ozlabs.org/patch/927116/ must be upated to
>> document this.
>> ---
>>  fs/common.mk | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>> 
>> diff --git a/fs/common.mk b/fs/common.mk
>> index 453da6010a..9c2f9c6dd4 100644
>> --- a/fs/common.mk
>> +++ b/fs/common.mk
>> @@ -106,6 +106,8 @@ rootfs-common-show-depends:
>>  # all variable references except the arguments must be $$-quoted.
>>  define inner-rootfs
>>  
>> +ROOTFS_$(2)_IMAGE_NAME ?= rootfs.$(1)
>> +ROOTFS_$(2)_FINAL_IMAGE_NAME = $$(call qstrip,$$(ROOTFS_$()_IMAGE_NAME))
>                                                           ^^^
> 
> This patch has not been tested at all, it can't even possibly work...

Pfff, I will send a follow-up fixing that. Sorry.

Scratching my head attempting to find how it happened.

[Mental note: stay away from recreational substances while at work].

-- 
Carlos Santos (Casantos) - DATACOM, P&D
?Marched towards the enemy, spear upright, armed with the certainty
that only the ignorant can have.? ? Epitaph of a volunteer

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH] fs: allow filesystems to set the name of their output file
  2018-10-24 14:36     ` Yann E. MORIN
  2018-10-25  0:20       ` [Buildroot] [PATCH v2] " Carlos Santos
@ 2018-11-03 10:37       ` Arnout Vandecappelle
  2018-11-03 13:34         ` Yann E. MORIN
  1 sibling, 1 reply; 15+ messages in thread
From: Arnout Vandecappelle @ 2018-11-03 10:37 UTC (permalink / raw)
  To: buildroot



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
>>
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH] fs: allow filesystems to set the name of their output file
  2018-11-03 10:37       ` [Buildroot] [PATCH] " Arnout Vandecappelle
@ 2018-11-03 13:34         ` Yann E. MORIN
  2018-11-03 22:09           ` Carlos Santos
  0 siblings, 1 reply; 15+ messages in thread
From: Yann E. MORIN @ 2018-11-03 13:34 UTC (permalink / raw)
  To: buildroot

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
> >>
> > 

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH] fs: allow filesystems to set the name of their output file
  2018-11-03 13:34         ` Yann E. MORIN
@ 2018-11-03 22:09           ` Carlos Santos
  2018-11-30 18:18             ` Yann E. MORIN
  0 siblings, 1 reply; 15+ messages in thread
From: Carlos Santos @ 2018-11-03 22:09 UTC (permalink / raw)
  To: buildroot

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.

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
Supersedes: https://patchwork.ozlabs.org/patch/988843/
Supersedes: https://patchwork.ozlabs.org/patch/988446/
---
Changes v1->v2:
- Prevent leading space when the filesystem gets 'imaginative', as
  pointed by Yann Morin.
Changes v2->v3:
- Fix typo and use strip instead of qstrip, as spotted by Yann Morin.

NOTE: https://patchwork.ozlabs.org/patch/927116/ must be upated to
document this.
---
 fs/common.mk | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/common.mk b/fs/common.mk
index 453da6010a..175c3aca13 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -106,6 +106,8 @@ rootfs-common-show-depends:
 # all variable references except the arguments must be $$-quoted.
 define inner-rootfs
 
+ROOTFS_$(2)_IMAGE_NAME ?= rootfs.$(1)
+ROOTFS_$(2)_FINAL_IMAGE_NAME = $$(call strip,$$(ROOTFS_$(2)_IMAGE_NAME))
 ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1)
 ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target
 
@@ -140,10 +142,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)_FINAL_IMAGE_NAME): ROOTFS=$(2)
+$$(BINARIES_DIR)/$$(ROOTFS_$(2)_FINAL_IMAGE_NAME): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
+$$(BINARIES_DIR)/$$(ROOTFS_$(2)_FINAL_IMAGE_NAME): $$(ROOTFS_$(2)_DEPENDENCIES)
+	@$$(call MESSAGE,"Generating filesystem image $$(ROOTFS_$(2)_FINAL_IMAGE_NAME)")
 	rm -rf $$(ROOTFS_$(2)_DIR)
 	mkdir -p $$(ROOTFS_$(2)_DIR)
 	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
@@ -164,7 +166,7 @@ endif
 rootfs-$(1)-show-depends:
 	@echo $$(ROOTFS_$(2)_DEPENDENCIES)
 
-rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1)
+rootfs-$(1): $$(BINARIES_DIR)/$$(ROOTFS_$(2)_FINAL_IMAGE_NAME)
 
 .PHONY: rootfs-$(1) rootfs-$(1)-show-depends
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH v2] fs: allow filesystems to set the name of their output file
  2018-10-25  0:20       ` [Buildroot] [PATCH v2] " Carlos Santos
  2018-11-01 21:01         ` Yann E. MORIN
@ 2018-11-03 22:13         ` Carlos Santos
  1 sibling, 0 replies; 15+ messages in thread
From: Carlos Santos @ 2018-11-03 22:13 UTC (permalink / raw)
  To: buildroot

Superseded-by: https://patchwork.ozlabs.org/patch/992684/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH v2] fs: allow filesystems to set the name of their output file
  2018-11-03  2:20           ` Carlos Santos
@ 2018-11-03 22:25             ` Carlos Santos
  0 siblings, 0 replies; 15+ messages in thread
From: Carlos Santos @ 2018-11-03 22:25 UTC (permalink / raw)
  To: buildroot

> From: "DATACOM" <casantos@datacom.com.br>
> To: "Yann Morin" <yann.morin.1998@free.fr>
> Cc: "buildroot" <buildroot@buildroot.org>
> Sent: Sexta-feira, 2 de novembro de 2018 23:20:06
> Subject: Re: [Buildroot] [PATCH v2] fs: allow filesystems to set the name of their output file

>> From: "Yann Morin" <yann.morin.1998@free.fr>
>> To: "DATACOM" <casantos@datacom.com.br>
>> Cc: "buildroot" <buildroot@buildroot.org>
>> Sent: Quinta-feira, 1 de novembro de 2018 18:01:03
>> Subject: Re: [PATCH v2] fs: allow filesystems to set the name of their output
>> file
> 
>>> +ROOTFS_$(2)_FINAL_IMAGE_NAME = $$(call qstrip,$$(ROOTFS_$()_IMAGE_NAME))
>>                                                           ^^^
>> 
>> This patch has not been tested at all, it can't even possibly work...
> 
> Pfff, I will send a follow-up fixing that. Sorry.
> 
> Scratching my head attempting to find how it happened.

FYI: I figured out the reason why the buggy patch was sent but I won't
tell ya because it is too much embarrassing. :-(

-- 
Carlos Santos (Casantos) - DATACOM, P&D
?Marched towards the enemy, spear upright, armed with the certainty
that only the ignorant can have.? ? Epitaph of a volunteer

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH] fs: allow filesystems to set the name of their output file
  2018-11-03 22:09           ` Carlos Santos
@ 2018-11-30 18:18             ` Yann E. MORIN
  0 siblings, 0 replies; 15+ messages in thread
From: Yann E. MORIN @ 2018-11-30 18:18 UTC (permalink / raw)
  To: buildroot

Carlos, All,

On 2018-11-03 19:09 -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.
> 
> Signed-off-by: Carlos Santos <casantos@datacom.com.br>

I was a bit reluctant at replying to this third iteration, but as Thomas
asked for my opinion on IRC, here is my review...

I'll say it blundly: this patch does not work. It can't possibly have
been tested as-is.

> ---
> Supersedes: https://patchwork.ozlabs.org/patch/988843/
> Supersedes: https://patchwork.ozlabs.org/patch/988446/
> ---
> Changes v1->v2:
> - Prevent leading space when the filesystem gets 'imaginative', as
>   pointed by Yann Morin.
> Changes v2->v3:
> - Fix typo and use strip instead of qstrip, as spotted by Yann Morin.
> 
> NOTE: https://patchwork.ozlabs.org/patch/927116/ must be upated to
> document this.
> ---
>  fs/common.mk | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/common.mk b/fs/common.mk
> index 453da6010a..175c3aca13 100644
> --- a/fs/common.mk
> +++ b/fs/common.mk
> @@ -106,6 +106,8 @@ rootfs-common-show-depends:
>  # all variable references except the arguments must be $$-quoted.
>  define inner-rootfs
>  
> +ROOTFS_$(2)_IMAGE_NAME ?= rootfs.$(1)
> +ROOTFS_$(2)_FINAL_IMAGE_NAME = $$(call strip,$$(ROOTFS_$(2)_IMAGE_NAME))

'strip' is not a macro, it is a built-in, so $(call)-ing it expands to
an empty string, so...

>  ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1)
>  ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target
>  
> @@ -140,10 +142,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)_FINAL_IMAGE_NAME): ROOTFS=$(2)
> +$$(BINARIES_DIR)/$$(ROOTFS_$(2)_FINAL_IMAGE_NAME): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
> +$$(BINARIES_DIR)/$$(ROOTFS_$(2)_FINAL_IMAGE_NAME): $$(ROOTFS_$(2)_DEPENDENCIES)

... these rules thus all have just $$(BINARIES_DIR) as their target, so
that doe not work very nicely...

> +	@$$(call MESSAGE,"Generating filesystem image $$(ROOTFS_$(2)_FINAL_IMAGE_NAME)")
>  	rm -rf $$(ROOTFS_$(2)_DIR)
>  	mkdir -p $$(ROOTFS_$(2)_DIR)
>  	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
> @@ -164,7 +166,7 @@ endif
>  rootfs-$(1)-show-depends:
>  	@echo $$(ROOTFS_$(2)_DEPENDENCIES)
>  
> -rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1)
> +rootfs-$(1): $$(BINARIES_DIR)/$$(ROOTFS_$(2)_FINAL_IMAGE_NAME)

... except maybe because of that?

Anyway, that is still an incorrect patch...

Regards,
Yann E. MORIN.

>  
>  .PHONY: rootfs-$(1) rootfs-$(1)-show-depends
>  
> -- 
> 2.17.1
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2018-11-30 18:18 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-22  2:31 [Buildroot] [RFC] fs: allow passing the image file name to inner-rootfs Carlos Santos
2018-10-21 13:26 ` Yann E. MORIN
2018-10-24  1:01   ` [Buildroot] [PATCH] fs: allow filesystems to set the name of their output file Carlos Santos
2018-10-24 14:36     ` Yann E. MORIN
2018-10-25  0:20       ` [Buildroot] [PATCH v2] " Carlos Santos
2018-11-01 21:01         ` Yann E. MORIN
2018-11-03  2:20           ` Carlos Santos
2018-11-03 22:25             ` Carlos Santos
2018-11-03 22:13         ` Carlos Santos
2018-11-03 10:37       ` [Buildroot] [PATCH] " Arnout Vandecappelle
2018-11-03 13:34         ` Yann E. MORIN
2018-11-03 22:09           ` Carlos Santos
2018-11-30 18:18             ` Yann E. MORIN
2018-10-25  0:23     ` Carlos Santos
2018-10-24  1:04   ` [Buildroot] [RFC] fs: allow passing the image file name to inner-rootfs Carlos Santos

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox