[Buildroot] [PATCH 0/2] support/scripts: add generic genimage wrapper

[Buildroot] [PATCH 0/2] support/scripts: add generic genimage wrapper

[Phelip Etienne]

Many boards are using the same genimage command in their post-image.sh
script.

This adds a generic wrapper for this command in support/scripts. The
post-image.sh for rapsberrypi script has been updated to use this
wrapper.

Another patch set will follow this one in order to update all boards
using genimage to use the wrapper.

Phelip Etienne (2):
  support/scripts: add generic genimage script
  board/raspberrypi: use generic genimage script

 board/raspberrypi/post-image.sh | 10 +---------
 support/scripts/genimage.sh     | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 9 deletions(-)
 create mode 100644 support/scripts/genimage.sh

-- 
2.9.3

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

[Phelip Etienne]

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

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
+if [ ! -n "$1" ]; then
+  usage >&2
+  echo "Error: Invalid argument!" >&2
+  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 $?
-- 
2.9.3

[Buildroot] [PATCH 2/2] board/raspberrypi: use generic genimage script

[Phelip Etienne]

This use the generic genimage wrapper for the raspberrypi instead of
calling genimage ourselves.

Signed-off-by: Phelip Etienne <etienne.phelip@savoirfairelinux.com>
---
 board/raspberrypi/post-image.sh | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/board/raspberrypi/post-image.sh b/board/raspberrypi/post-image.sh
index b2bb070..a58fa11 100755
--- a/board/raspberrypi/post-image.sh
+++ b/board/raspberrypi/post-image.sh
@@ -3,7 +3,6 @@
 BOARD_DIR="$(dirname $0)"
 BOARD_NAME="$(basename ${BOARD_DIR})"
 GENIMAGE_CFG="${BOARD_DIR}/genimage-${BOARD_NAME}.cfg"
-GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp"
 
 case "${2}" in
 	--add-pi3-miniuart-bt-overlay)
@@ -18,13 +17,6 @@ __EOF__
 	;;
 esac
 
-rm -rf "${GENIMAGE_TMP}"
-
-genimage                           \
-	--rootpath "${TARGET_DIR}"     \
-	--tmppath "${GENIMAGE_TMP}"    \
-	--inputpath "${BINARIES_DIR}"  \
-	--outputpath "${BINARIES_DIR}" \
-	--config "${GENIMAGE_CFG}"
+sh ./support/scripts/genimage.sh ${GENIMAGE_CFG}
 
 exit $?
-- 
2.9.3

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

[Arnout Vandecappelle]

 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

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 2/2] board/raspberrypi: use generic genimage script

[Arnout Vandecappelle]

On 27-03-17 18:47, Phelip Etienne wrote:
> This use the generic genimage wrapper for the raspberrypi instead of
> calling genimage ourselves.
> 
> Signed-off-by: Phelip Etienne <etienne.phelip@savoirfairelinux.com>
> ---
>  board/raspberrypi/post-image.sh | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/board/raspberrypi/post-image.sh b/board/raspberrypi/post-image.sh
> index b2bb070..a58fa11 100755
> --- a/board/raspberrypi/post-image.sh
> +++ b/board/raspberrypi/post-image.sh
> @@ -3,7 +3,6 @@
>  BOARD_DIR="$(dirname $0)"
>  BOARD_NAME="$(basename ${BOARD_DIR})"
>  GENIMAGE_CFG="${BOARD_DIR}/genimage-${BOARD_NAME}.cfg"
> -GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp"
>  
>  case "${2}" in
>  	--add-pi3-miniuart-bt-overlay)

 Since this part is in fact only used by raspberrypi3_defconfig, you can
actually change all other raspberrypi?_defconfigs to call genimage.sh directly,
and use this script only for raspberry3. Then the argument is no longer needed
either.

 Regards,
 Arnout

> @@ -18,13 +17,6 @@ __EOF__
>  	;;
>  esac
>  
> -rm -rf "${GENIMAGE_TMP}"
> -
> -genimage                           \
> -	--rootpath "${TARGET_DIR}"     \
> -	--tmppath "${GENIMAGE_TMP}"    \
> -	--inputpath "${BINARIES_DIR}"  \
> -	--outputpath "${BINARIES_DIR}" \
> -	--config "${GENIMAGE_CFG}"
> +sh ./support/scripts/genimage.sh ${GENIMAGE_CFG}
>  
>  exit $?
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

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

[Étienne Phélip]

Hello Arnout!

----- Le 27 Mar 17, ? 16:40, Arnout Vandecappelle arnout at 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

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

[Arnout Vandecappelle]

On 27-03-17 23:06, ?tienne Ph?lip wrote:
> Hello Arnout!
> 
> ----- Le 27 Mar 17, ? 16:40, Arnout Vandecappelle arnout at 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
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF