[Buildroot] [RFC 0/4] fix genimage wrapper argument parsing

[Buildroot] [RFC 0/4] fix genimage wrapper argument parsing

[Etienne Phelip]

The genimage wrapper can be used in the post-image script but 
BR2_ROOTFS_POST_SCRIPT_ARGS passes BINARIES_DIR as the first argument.

In order to make genimage.sh play nicely with
BR2_ROOTFS_POST_IMAGE_SCRIPT, this patch series shifts the first
argument by one before looping over the other arguments.

When calling genimage.sh directly, one should always pass BINARIES_DIR
as the first argument like:

    ${TOPDIR}/support/scripts/genimage.sh ${BINARIES_DIR} \
        -c path/to/genimage.cfg

Etienne Phelip (4):
  genimage.sh: fix argument parsing
  genimage.sh: fix getopts traces
  genimage.sh: strip getopts EOL characters
  genimage.sh: add usage to die function

 support/scripts/genimage.sh | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

-- 
2.9.3

[Buildroot] [RFC 1/4] genimage.sh: fix argument parsing

[Etienne Phelip]

When executed as a post-image script, BR sets BINARIE_DIR as first
argument.

This first argument causes the option parsing to break.

This patch shifts the first argument before performing the getopts
loop.

	BR2_ROOTFS_POST_IMAGE_SCRIPT="support/scripts/genimage.sh"
	BR2_ROOTFS_POST_SCRIPT_ARGS="-c path/to/genimage.cfg"

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

diff --git a/support/scripts/genimage.sh b/support/scripts/genimage.sh
index 0ed0e8b..6684e73 100755
--- a/support/scripts/genimage.sh
+++ b/support/scripts/genimage.sh
@@ -7,6 +7,9 @@ die() {
 
 GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp"
 
+# First argument is BINARIES_DIR, skip it
+shift
+
 while getopts c: OPT ; do
 	case "${OPT}" in
 	c) GENIMAGE_CFG="${OPTARG}";;
-- 
2.9.3

[Buildroot] [RFC 2/4] genimage.sh: fix getopts traces

[Etienne Phelip]

The first trace is never displayed, the second shows an empty string
because OPTARG is not set.

To display both traces properly, the optstring of getopts needs to
start with a colon.

Signed-off-by: Etienne Phelip <etienne.phelip@savoirfairelinux.com>
---
 support/scripts/genimage.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/support/scripts/genimage.sh b/support/scripts/genimage.sh
index 6684e73..6f9102c 100755
--- a/support/scripts/genimage.sh
+++ b/support/scripts/genimage.sh
@@ -10,7 +10,7 @@ GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp"
 # First argument is BINARIES_DIR, skip it
 shift
 
-while getopts c: OPT ; do
+while getopts :c: OPT ; do
 	case "${OPT}" in
 	c) GENIMAGE_CFG="${OPTARG}";;
 	:) die "option '${OPTARG}' expects a mandatory argument\n";;
-- 
2.9.3

[Buildroot] [RFC 3/4] genimage.sh: strip getopts EOL characters

[Etienne Phelip]

The \n polutes the output.

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

diff --git a/support/scripts/genimage.sh b/support/scripts/genimage.sh
index 6f9102c..50d6907 100755
--- a/support/scripts/genimage.sh
+++ b/support/scripts/genimage.sh
@@ -13,8 +13,8 @@ shift
 while getopts :c: OPT ; do
 	case "${OPT}" in
 	c) GENIMAGE_CFG="${OPTARG}";;
-	:) die "option '${OPTARG}' expects a mandatory argument\n";;
-	\?) die "unknown option '${OPTARG}'\n";;
+	:) die "option '${OPTARG}' expects a mandatory argument";;
+	\?) die "unknown option '${OPTARG}'";;
 	esac
 done
 
-- 
2.9.3

[Buildroot] [RFC 4/4] genimage.sh: add usage to die function

[Etienne Phelip]

This also re-indents the die function using tabulations.

Signed-off-by: Etienne Phelip <etienne.phelip@savoirfairelinux.com>
---
 support/scripts/genimage.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/support/scripts/genimage.sh b/support/scripts/genimage.sh
index 50d6907..d0d3491 100755
--- a/support/scripts/genimage.sh
+++ b/support/scripts/genimage.sh
@@ -1,8 +1,11 @@
 #!/bin/bash
 
 die() {
-  echo "Error: $@" >&2
-  exit 1
+	echo "Usage: ${0##*/} BINARIES_DIR -c GENIMAGE_CFG" >&2
+	echo "" >&2
+
+	echo "Error: $@" >&2
+	exit 1
 }
 
 GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp"
@@ -18,7 +21,7 @@ while getopts :c: OPT ; do
 	esac
 done
 
-[ -n "${GENIMAGE_CFG}" ] || die "Missing argument"
+[ -n "${GENIMAGE_CFG}" ] || die "missing option 'c'"
 
 rm -rf "${GENIMAGE_TMP}"
 
-- 
2.9.3

[Buildroot] [RFC 0/4] fix genimage wrapper argument parsing

[Arnout Vandecappelle]

 Hi Etienne,

On 18-04-17 17:55, Etienne Phelip wrote:
> The genimage wrapper can be used in the post-image script but 
> BR2_ROOTFS_POST_SCRIPT_ARGS passes BINARIES_DIR as the first argument.
> 
> In order to make genimage.sh play nicely with
> BR2_ROOTFS_POST_IMAGE_SCRIPT, this patch series shifts the first
> argument by one before looping over the other arguments.
> 
> When calling genimage.sh directly, one should always pass BINARIES_DIR
> as the first argument like:
> 
>     ${TOPDIR}/support/scripts/genimage.sh ${BINARIES_DIR} \
>         -c path/to/genimage.cfg

 I like Abhimanyu's solution [1] better, because it makes passing BINARIES_DIR
optional and you can use it either directly as a post-image script or call it
from your custom post-image script. I did make a few comments in [1], but you
may want to have a look at it too and see if there is anything else to improve.

 That invalidates patch 1-3. Patch 4 still applies though and is AFAICS
independent of the rest. And it applies with a bit of fuzz on top of Abhimanyu's
patch, so I'll ack that one. Patch 1-3 I'll mark as rejected in Patchwork - if
you disagree, we can always re-instate them or you can resend.

 Regards,
 Arnout

[1] http://patchwork.ozlabs.org/patch/751165/

> 
> Etienne Phelip (4):
>   genimage.sh: fix argument parsing
>   genimage.sh: fix getopts traces
>   genimage.sh: strip getopts EOL characters
>   genimage.sh: add usage to die function
> 
>  support/scripts/genimage.sh | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 

-- 
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] [RFC 4/4] genimage.sh: add usage to die function

[Arnout Vandecappelle]

 Ah, sorry, I won't ack it :-)

On 18-04-17 17:55, Etienne Phelip wrote:
> This also re-indents the die function using tabulations.
> 
> Signed-off-by: Etienne Phelip <etienne.phelip@savoirfairelinux.com>
> ---
>  support/scripts/genimage.sh | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/support/scripts/genimage.sh b/support/scripts/genimage.sh
> index 50d6907..d0d3491 100755
> --- a/support/scripts/genimage.sh
> +++ b/support/scripts/genimage.sh
> @@ -1,8 +1,11 @@
>  #!/bin/bash
>  
>  die() {
> -  echo "Error: $@" >&2
> -  exit 1
> +	echo "Usage: ${0##*/} BINARIES_DIR -c GENIMAGE_CFG" >&2
> +	echo "" >&2
> +

 This empty line is not needed IMO.

> +	echo "Error: $@" >&2

 IMO Error should come first, then usage.

 And of course, to be compatible with Abhimanyu's patch, BINARIES_DIR should be
left out.

 Regards,
 Arnout

> +	exit 1
>  }
>  
>  GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp"
> @@ -18,7 +21,7 @@ while getopts :c: OPT ; do
>  	esac
>  done
>  
> -[ -n "${GENIMAGE_CFG}" ] || die "Missing argument"
> +[ -n "${GENIMAGE_CFG}" ] || die "missing option 'c'"
>  
>  rm -rf "${GENIMAGE_TMP}"
>  
> 

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