[RFC-next,1/2] fs/genimage: add logic to generate an image using genimage

Message ID 20170823204547.16672-2-thomas.petazzoni@free-electrons.com
State Changes Requested
Headers show

Commit Message

Thomas Petazzoni Aug. 23, 2017, 8:45 p.m.
We have more and more boards that need to repeat in their post-image
script the logic to use genimage. Even though the logic has been
factorized in support/scripts/genimage.sh, users still have to
remember that they need to enable BR2_PACKAGE_HOST_MTOOLS and
BR2_PACKAGE_HOST_DOSFSTOOLS when they have a VFAT partition in their
genimage configuration.

Therefore, this commit adds some minimal logic in Buildroot to use
genimage directly.

This logic is added into fs/genimage/, even if stricly speaking
genimage isn't a filesystem format. It does not use the filesystem
infrastructure (fs/common.mk), and is referenced explicitly from the
target-post-image target in the main Makefile.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 Makefile                |  1 +
 fs/Config.in            |  1 +
 fs/genimage/Config.in   | 26 ++++++++++++++++++++++++++
 fs/genimage/genimage.mk | 31 +++++++++++++++++++++++++++++++
 4 files changed, 59 insertions(+)
 create mode 100644 fs/genimage/Config.in
 create mode 100644 fs/genimage/genimage.mk

Comments

Arnout Vandecappelle Aug. 24, 2017, 11:33 p.m. | #1
On 23-08-17 22:45, Thomas Petazzoni wrote:
> We have more and more boards that need to repeat in their post-image
> script the logic to use genimage. 

 Perhaps you should add least one patch to your series that updates a defconfig
with this feature.


> Even though the logic has been
> factorized in support/scripts/genimage.sh, users still have to
> remember that they need to enable BR2_PACKAGE_HOST_MTOOLS and
> BR2_PACKAGE_HOST_DOSFSTOOLS when they have a VFAT partition in their
> genimage configuration.
> 
> Therefore, this commit adds some minimal logic in Buildroot to use
> genimage directly.
> 
> This logic is added into fs/genimage/, even if stricly speaking
> genimage isn't a filesystem format. It does not use the filesystem
> infrastructure (fs/common.mk), and is referenced explicitly from the
> target-post-image target in the main Makefile.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
[snip]
> diff --git a/fs/genimage/genimage.mk b/fs/genimage/genimage.mk
> new file mode 100644
> index 0000000..b44b29b
> --- /dev/null
> +++ b/fs/genimage/genimage.mk
> @@ -0,0 +1,31 @@
> +################################################################################
> +#
> +# genimage support
> +#
> +################################################################################
> +
> +ifeq ($(BR2_TARGET_ROOTFS_GENIMAGE),y)
> +PACKAGES += host-genimage

 This shouldn't be needed, since Config.in already adds them to the packages
list, no?

> +
> +ifeq ($(BR2_TARGET_ROOTFS_GENIMAGE_USES_VFAT),y)
> +PACKAGES += host-dosfstools host-mtools

 Same here.

> +endif
> +
> +GENIMAGE_CFG_FILE = $(call qstrip,$(BR2_TARGET_ROOTFS_GENIMAGE_CFG_FILE))

 Perhaps we should $(error) if the file doesn't exist? Hm, hangon, the
genimage.cfg file could be generated by a post-{build,image} script, so no,
don't error here...

> +GENIMAGE_TMP = $(BUILD_DIR)/genimage.tmp
> +
> +define GENIMAGE_GENERATE_CMDS
> +	if grep -q "vfat.*{" $(GENIMAGE_CFG_FILE) && \

 Unfortunately whitespace is not significant in genimage files, so it is
possible that this regex matches in a genimage.cfg that doesn't have a vfat
partition at all but just has the string "vfat" somewhere... OK, that's a bit a
weird situation.

> +	   test "$(BR2_TARGET_ROOTFS_GENIMAGE_USES_VFAT)" != "y" ; then \
> +		echo "ERROR: genimage configuration generates a VFAT filesystem" ; \
> +		echo "       please enable BR2_TARGET_ROOTFS_GENIMAGE_USES_VFAT" ; \
> +	fi
> +	$(RM) -rf $(GENIMAGE_TMP)
> +	$(HOST_DIR)/usr/bin/genimage --rootpath $(TARGET_DIR) \
> +		--tmppath $(GENIMAGE_TMP) \
> +		--inputpath $(BINARIES_DIR) \
> +		--outputpath $(BINARIES_DIR) \
> +		--config $(GENIMAGE_CFG_FILE)

 So the idea is to drop the genimage.sh script entirely? Hm, in the light of the
longer-term vision, I think things will become more and more complicated so
delegating it to a script sounds like the right thing to do...

 Regards,
 Arnout

> +endef
> +
> +endif # BR2_TARGET_ROOTFS_GENIMAGE
>
Ricardo Martincoski Aug. 26, 2017, 9:08 p.m. | #2
Hello,

On Thu, Aug 24, 2017 at 08:33 PM, Arnout Vandecappelle wrote:

> On 23-08-17 22:45, Thomas Petazzoni wrote:
[snip]
>> +	   test "$(BR2_TARGET_ROOTFS_GENIMAGE_USES_VFAT)" != "y" ; then \
>> +		echo "ERROR: genimage configuration generates a VFAT filesystem" ; \
>> +		echo "       please enable BR2_TARGET_ROOTFS_GENIMAGE_USES_VFAT" ; \
>> +	fi
>> +	$(RM) -rf $(GENIMAGE_TMP)
>> +	$(HOST_DIR)/usr/bin/genimage --rootpath $(TARGET_DIR) \

I think you need to pass $(EXTRA_ENV) or a subset of it here, otherwise it will
not use mkdosfs from the host/ directory and will fail if it not pre-installed,
see:
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/30257871
sh: 1: mkdosfs: not found

>> +		--tmppath $(GENIMAGE_TMP) \
>> +		--inputpath $(BINARIES_DIR) \
>> +		--outputpath $(BINARIES_DIR) \
>> +		--config $(GENIMAGE_CFG_FILE)
> 
>  So the idea is to drop the genimage.sh script entirely? Hm, in the light of the
> longer-term vision, I think things will become more and more complicated so
> delegating it to a script sounds like the right thing to do...

Even if the script is called I guess $(EXTRA_ENV) will be needed, just like
target-post-image already does for custom scripts.

Regards,
Ricardo

Patch

diff --git a/Makefile b/Makefile
index 1b0e36f..3baad7b 100644
--- a/Makefile
+++ b/Makefile
@@ -733,6 +733,7 @@  endif
 
 .PHONY: target-post-image
 target-post-image: $(TARGETS_ROOTFS) target-finalize
+	$(GENIMAGE_GENERATE_CMDS)
 	@$(foreach s, $(call qstrip,$(BR2_ROOTFS_POST_IMAGE_SCRIPT)), \
 		$(call MESSAGE,"Executing post-image script $(s)"); \
 		$(EXTRA_ENV) $(s) $(BINARIES_DIR) $(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS))$(sep))
diff --git a/fs/Config.in b/fs/Config.in
index 51ccf28..04b4ebe 100644
--- a/fs/Config.in
+++ b/fs/Config.in
@@ -13,5 +13,6 @@  source "fs/squashfs/Config.in"
 source "fs/tar/Config.in"
 source "fs/ubifs/Config.in"
 source "fs/yaffs2/Config.in"
+source "fs/genimage/Config.in"
 
 endmenu
diff --git a/fs/genimage/Config.in b/fs/genimage/Config.in
new file mode 100644
index 0000000..bd747bc
--- /dev/null
+++ b/fs/genimage/Config.in
@@ -0,0 +1,26 @@ 
+config BR2_TARGET_ROOTFS_GENIMAGE
+	bool "generate a final image with genimage"
+	select BR2_PACKAGE_HOST_GENIMAGE
+	help
+	  This option allows to generate a final image to flash on
+	  your device using a tool called genimage.
+
+if BR2_TARGET_ROOTFS_GENIMAGE
+
+config BR2_TARGET_ROOTFS_GENIMAGE_CFG_FILE
+	string "genimage config file"
+	help
+	  This option allows to specify the path to the genimage
+	  configuration file. See
+	  https://git.pengutronix.de/cgit/genimage/plain/README for
+	  the specification of the genimage configuration file format.
+
+config BR2_TARGET_ROOTFS_GENIMAGE_USES_VFAT
+	bool "genimage config uses vfat"
+	select BR2_PACKAGE_HOST_DOSFSTOOLS
+	select BR2_PACKAGE_HOST_MTOOLS
+	help
+	  Enable this option if your genimage configuration file uses
+	  a "vfat" section to generate a VFAT filesystem.
+
+endif
diff --git a/fs/genimage/genimage.mk b/fs/genimage/genimage.mk
new file mode 100644
index 0000000..b44b29b
--- /dev/null
+++ b/fs/genimage/genimage.mk
@@ -0,0 +1,31 @@ 
+################################################################################
+#
+# genimage support
+#
+################################################################################
+
+ifeq ($(BR2_TARGET_ROOTFS_GENIMAGE),y)
+PACKAGES += host-genimage
+
+ifeq ($(BR2_TARGET_ROOTFS_GENIMAGE_USES_VFAT),y)
+PACKAGES += host-dosfstools host-mtools
+endif
+
+GENIMAGE_CFG_FILE = $(call qstrip,$(BR2_TARGET_ROOTFS_GENIMAGE_CFG_FILE))
+GENIMAGE_TMP = $(BUILD_DIR)/genimage.tmp
+
+define GENIMAGE_GENERATE_CMDS
+	if grep -q "vfat.*{" $(GENIMAGE_CFG_FILE) && \
+	   test "$(BR2_TARGET_ROOTFS_GENIMAGE_USES_VFAT)" != "y" ; then \
+		echo "ERROR: genimage configuration generates a VFAT filesystem" ; \
+		echo "       please enable BR2_TARGET_ROOTFS_GENIMAGE_USES_VFAT" ; \
+	fi
+	$(RM) -rf $(GENIMAGE_TMP)
+	$(HOST_DIR)/usr/bin/genimage --rootpath $(TARGET_DIR) \
+		--tmppath $(GENIMAGE_TMP) \
+		--inputpath $(BINARIES_DIR) \
+		--outputpath $(BINARIES_DIR) \
+		--config $(GENIMAGE_CFG_FILE)
+endef
+
+endif # BR2_TARGET_ROOTFS_GENIMAGE