[1/2] support/scripts: add generic genimage script

This script is a wrapper for the genimage tool used by most boards.
The board postimage script can now call this script instead of invoking
genimage command themselves.

Signed-off-by: Phelip Etienne <etienne.phelip@savoirfairelinux.com>
---
 support/scripts/genimage.sh | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 support/scripts/genimage.sh

Hi Phelip,

 Nice to see yet another contribution from Savoir-faire!

On 27-03-17 18:47, Phelip Etienne wrote:
> This script is a wrapper for the genimage tool used by most boards.
> The board postimage script can now call this script instead of invoking
> genimage command themselves.

 Looks good except for some small comments below. It would have been nice,
however, if you had included a patch in this series that completely removes one
of the post-image scripts. Almost all of them can be removed completely,
raspberrypi is one of the exceptions...

> 
> Signed-off-by: Phelip Etienne <etienne.phelip@savoirfairelinux.com>
> ---
>  support/scripts/genimage.sh | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 support/scripts/genimage.sh
> 
> diff --git a/support/scripts/genimage.sh b/support/scripts/genimage.sh
> new file mode 100644
> index 0000000..134fc98
> --- /dev/null
> +++ b/support/scripts/genimage.sh
> @@ -0,0 +1,26 @@
> +#!/bin/sh
> +
> +usage() {
> +  echo "Usage: genimage.sh GENIMAGE_CFG"
> +}
> +
> +# Exit if argument is missing

 Comment is redundant.

> +if [ ! -n "$1" ]; then
> +  usage >&2
> +  echo "Error: Invalid argument!" >&2

 It's a bit nicer to move the >&2 and the error message to usage(), so you can
just call it as
  usage "Error: Invalid argument!"
and actually, it's "Missing argument", not invalid.

 Also, first print the error message, then the usage itself.

> +  exit 1
> +fi
> +
> +GENIMAGE_CFG=$1
> +GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp"
> +
> +rm -rf "${GENIMAGE_TMP}"
> +
> +genimage  \
> +	--rootpath "${TARGET_DIR}"     \
> +	--tmppath "${GENIMAGE_TMP}"    \
> +	--inputpath "${BINARIES_DIR}"  \
> +	--outputpath "${BINARIES_DIR}" \
> +	--config "${GENIMAGE_CFG}"
> +
> +exit $?

 The exit is not needed - the script will exit with the exit code of the last
command. (Almost) all other post-image scripts don't have it.

 Regards,
 Arnout

Hello Arnout!

----- Le 27 Mar 17, à 16:40, Arnout Vandecappelle arnout@mind.be a écrit :

> Hi Phelip,
> 
> Nice to see yet another contribution from Savoir-faire!
> 
> On 27-03-17 18:47, Phelip Etienne wrote:
>> This script is a wrapper for the genimage tool used by most boards.
>> The board postimage script can now call this script instead of invoking
>> genimage command themselves.
> 
> Looks good except for some small comments below. It would have been nice,
> however, if you had included a patch in this series that completely removes one
> of the post-image scripts. Almost all of them can be removed completely,
> raspberrypi is one of the exceptions...

Good, since you seem to agree with the proposed changes, I will respin
a new patch series addressing your comments, as well as applying the
change to all other boards using genimage.

In the meantime, I plan to add a Kconfig entry to specify a list of
space separated genimage config files and make Buildroot call the
wrapper automatically (after the post-image script), for each config
file. Do you think I should add this Kconfig/Makefile addition
in a future series, or directly in this one?

Thanks,
--Etienne

On 27-03-17 23:06, Étienne Phélip wrote:
> Hello Arnout!
> 
> ----- Le 27 Mar 17, à 16:40, Arnout Vandecappelle arnout@mind.be a écrit :
> 
>> Hi Phelip,
>>
>> Nice to see yet another contribution from Savoir-faire!
>>
>> On 27-03-17 18:47, Phelip Etienne wrote:
>>> This script is a wrapper for the genimage tool used by most boards.
>>> The board postimage script can now call this script instead of invoking
>>> genimage command themselves.
>>
>> Looks good except for some small comments below. It would have been nice,
>> however, if you had included a patch in this series that completely removes one
>> of the post-image scripts. Almost all of them can be removed completely,
>> raspberrypi is one of the exceptions...
> 
> Good, since you seem to agree with the proposed changes, I will respin
> a new patch series addressing your comments, as well as applying the
> change to all other boards using genimage.

 I had one more idea for the script: perhaps it's better to use an explicit
option for the config argument, e.g. --genimage-cfg=... . Remember that the same
argument list is used for the post-build, post-fakeroot and post-image scripts.
So relying on the order of arguments is difficult if several scripts are used.
Look at the a20_olinuxino scripts for examples.


> In the meantime, I plan to add a Kconfig entry to specify a list of
> space separated genimage config files and make Buildroot call the
> wrapper automatically (after the post-image script), for each config

 Hm, before or after the post-image script? It's possible that the post-image
script generates something used by genimage (e.g. some postprocessing of the
kernel or bootloader image; cfr. firefly-rk3288), but it's also possible that
the post-image script does something with the result of genimage (e.g. writing
the bootloader to the start of the image, cfr. odroidc2, or writing the image to
an SD card).

> file. Do you think I should add this Kconfig/Makefile addition
> in a future series, or directly in this one?

 Well, if you first convert all defconfigs to the generic genimage script, and
then convert them again to use the new Kconfig option, that generates a lot of
useless churn. So if you have the patience, I think it's better to now post an
RFC series that also introduces the new Kconfig option and then converts one or
two defconfigs to show how it goes.

 Regards,
 Arnout

> 
> Thanks,
> --Etienne
>