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

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

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
>

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