| Message ID | 20170416174048.4737-1-abhimanyu.v@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Hello Abhimanyu, On Sun, Apr 16, 2017 at 11:10:47PM +0530, Abhimanyu Vishwakarma wrote: > From: Abhimanyu Vishwakarma <Abhimanyu.V@gmail.com> > > When called from BR2_ROOTFS_POST_IMAGE_SCRIPT, this script > ends up with following error: > > Error: Missing argument > > This is because, extra positional argument is also passed > along with BR2_ROOTFS_POST_SCRIPT_ARGS. genimage.sh didnt > had support to parse positional and optional argument > together. > Indeed, the problem comes from the first argument given to any post-image scripts [1] (which is the binary directory). A more simple fix consists in adding the shift instruction before right before the while getopts. + 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";; esac done > Signed-off-by: Abhimanyu Vishwakarma <Abhimanyu.V@gmail.com> > --- > Changes v7->v8 > - New file > > support/scripts/genimage.sh | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/support/scripts/genimage.sh b/support/scripts/genimage.sh > index 0ed0e8bcc..2b5549fb0 100755 > --- a/support/scripts/genimage.sh > +++ b/support/scripts/genimage.sh > @@ -5,13 +5,18 @@ die() { > exit 1 > } > > +# Parse arguments and put into argument list of the script > +eval set -- $(getopt -n genimage.sh -o c: -- "$@") > + > GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp" > > -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";; > +while true ; do > + case "$1" in > + -c) [ ! -z "$2" ] || die "option '${1}' expects a mandatory argument\n"; > + GENIMAGE_CFG="${2}"; > + shift 2 ;; > + --) shift 1; break ;; > + *) die "unknown option '${1}'\n";; > esac > done > > -- > 2.11.0 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot Furthermore, I see an error in traces. To display the traces properly, the optstring required to start with a colon (:), and the trailling \n is not required. while getopts :c: OPT ; do [1] https://github.com/buildroot/buildroot/blob/2017.02.1/Makefile#L717 Regards, Gaël
Thankyou Gaël for review! On Mon, Apr 17, 2017 at 3:08 AM, Gaël PORTAY < gael.portay@savoirfairelinux.com> wrote: > Hello Abhimanyu, > > On Sun, Apr 16, 2017 at 11:10:47PM +0530, Abhimanyu Vishwakarma wrote: > > From: Abhimanyu Vishwakarma <Abhimanyu.V@gmail.com> > > > > When called from BR2_ROOTFS_POST_IMAGE_SCRIPT, this script > > ends up with following error: > > > > Error: Missing argument > > > > This is because, extra positional argument is also passed > > along with BR2_ROOTFS_POST_SCRIPT_ARGS. genimage.sh didnt > > had support to parse positional and optional argument > > together. > > > > Indeed, the problem comes from the first argument given to any post-image > scripts [1] (which is the binary directory). > > A more simple fix consists in adding the shift instruction before right > before > the while getopts. > > + 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";; > esac > done > > Yes you are right, this is simple fix. What i had in mind was to make the script independent of any positional argument. With proposed solution if in future we add extra argument to POST_SCRIPTS then this script can cope with it. Please let me know your thought. I will prepare patch accordingly. <snip> > Furthermore, I see an error in traces. To display the traces properly, the > optstring required to start with a colon (:), and the trailling \n is not > required. > > while getopts :c: OPT ; do > > Thanks, I will fix it. > [1] https://github.com/buildroot/buildroot/blob/2017.02.1/Makefile#L717 > > Regards, > Gaël > Regards Abhimanyu V
Hello Abhimanyu, On Mon, Apr 17, 2017 at 03:38:27PM +0530, abhimanyu.v@gmail.com wrote: > Thankyou Gaël for review! > > [...] > > > > Indeed, the problem comes from the first argument given to any post-image > > scripts [1] (which is the binary directory). > > > > A more simple fix consists in adding the shift instruction before right > > before > > the while getopts. > > > > + 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";; > > esac > > done > > > > > Yes you are right, this is simple fix. What i had in mind was to make the > script independent of any positional argument. With proposed solution if in > future we add extra argument to POST_SCRIPTS then this script can cope with > it. Please let me know your thought. I will prepare patch accordingly. > > <snip> > Afterward, my suggestion about shift does not suit me. If genimage.sh is run inside a post-image script, this same script have to give a fake parameter as first argument. #!/bin/sh # # BR post-image script # # (...) support/scripts/genimage.sh "$1" -c path/to/genimage.cfg Initially, genimage.sh script was NOT intend to be run either as or inside a post-image. It was a Makefile target [1]. [2] and [3] are use cases. Maybe, a cleaner solution consists in updating the Makefile to remove this first argument given to both post-build and post-image scripts. But it breaks the existing. Thomas, Arnout, do you have a better idea? I had a quick look to scripts in-tree; they do not seem to use this parameter. Instead, they access directly to $TARGET_DIR or $BINARIES_DIR values using the environment variables. For extra arguments, they use $2, $3; they need to be updated. > > Furthermore, I see an error in traces. To display the traces properly, the > > optstring required to start with a colon (:), and the trailling \n is not > > required. > > > > while getopts :c: OPT ; do > > > > > Thanks, I will fix it. > > > > [1] https://github.com/buildroot/buildroot/blob/2017.02.1/Makefile#L717 > > > > Regards, > > Gaël > > > > Regards > Abhimanyu V > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot Regards, Gaël [1] http://patchwork.ozlabs.org/patch/744825/ [2] http://patchwork.ozlabs.org/patch/744826/ [3] http://patchwork.ozlabs.org/patch/744824/>
Hello Gaël, On Mon, Apr 17, 2017 at 7:50 PM, Gaël PORTAY < gael.portay@savoirfairelinux.com> wrote: > Hello Abhimanyu, > > On Mon, Apr 17, 2017 at 03:38:27PM +0530, abhimanyu.v@gmail.com wrote: > > Thankyou Gaël for review! > > > <snip> > Afterward, my suggestion about shift does not suit me. > > If genimage.sh is run inside a post-image script, this same script have to > give > a fake parameter as first argument. > > #!/bin/sh > # > # BR post-image script > # > # (...) > > support/scripts/genimage.sh "$1" -c path/to/genimage.cfg > > The proposed changes to script doesn't have above issue. <snip> Regards Abhimanyu V
On 16-04-17 19:40, Abhimanyu Vishwakarma wrote: > From: Abhimanyu Vishwakarma <Abhimanyu.V@gmail.com> > > When called from BR2_ROOTFS_POST_IMAGE_SCRIPT, this script > ends up with following error: > > Error: Missing argument > > This is because, extra positional argument is also passed > along with BR2_ROOTFS_POST_SCRIPT_ARGS. genimage.sh didnt > had support to parse positional and optional argument > together. > > Signed-off-by: Abhimanyu Vishwakarma <Abhimanyu.V@gmail.com> > --- > Changes v7->v8 > - New file > > support/scripts/genimage.sh | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/support/scripts/genimage.sh b/support/scripts/genimage.sh > index 0ed0e8bcc..2b5549fb0 100755 > --- a/support/scripts/genimage.sh > +++ b/support/scripts/genimage.sh > @@ -5,13 +5,18 @@ die() { > exit 1 > } > > +# Parse arguments and put into argument list of the script > +eval set -- $(getopt -n genimage.sh -o c: -- "$@") The proper way is to quote the $(getopt ...) part, because the eval will do an additional level of unquoting. Otherwise if an option contains a shell meta character (e.g. $) it will be expanded by eval. Also, instead of explicitly putting genimage.sh, it's better to pass the basename with which it was called. Also, getopt will print an error message and return false if something is wrong with the options, so better handle that. So: opts="$(getopt -n "${0##*/}" -o c: -- "$@")" || exit $? eval set -- "$opts" > + > GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp" > > -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";; > +while true ; do > + case "$1" in > + -c) [ ! -z "$2" ] || die "option '${1}' expects a mandatory argument\n"; This check shouldn't be needed since it's already done by getopt. > + GENIMAGE_CFG="${2}"; > + shift 2 ;; > + --) shift 1; break ;; This could use a comment, e.g. # Discard all non-option parameters Also, since you use one line per statement above, do it here as well. In fact, I'm personally in favour of splitting off the case part (i.e., -c) and --) ) on a separate line as well. > + *) die "unknown option '${1}'\n";; This is not strictly needed since getopt will already detect it, but I guess better to keep it as an emergency fallback. Regards, Arnout > esac > done > >
[Adding Etienne to the Cc as the main contributor of genimage.sh] On 17-04-17 16:20, Gaël PORTAY wrote: > Hello Abhimanyu, > [snip] > Initially, genimage.sh script was NOT intend to be run either as or inside a > post-image. It was a Makefile target [1]. [2] and [3] are use cases. True, but since [1] isn't applied yet and is still under discussion, we really want to be able to call it as a post-image script directly. > Maybe, a cleaner solution consists in updating the Makefile to remove this first > argument given to both post-build and post-image scripts. But it breaks the > existing. I don't think we can do that. Most "interesting" post-* scripts are out of tree, so we can't change them. Although updating Buildroot is expected to have some implications on the user integration layer (cfr. the change in br2-external handling), we try to avoid it. It would indeed be good to remove that first argument, but then I think we have to go through a (long) deprecation period. That means right now: mark it as legacy in the manual and help text. > Thomas, Arnout, do you have a better idea? > > I had a quick look to scripts in-tree; they do not seem to use this parameter. > > Instead, they access directly to $TARGET_DIR or $BINARIES_DIR values using the > environment variables. > > For extra arguments, they use $2, $3; they need to be updated. ... which proves that deprecating it is not easy, because it's fairly difficult to make a script compatible with both the "new" and the "old" way - cfr. the special care taking in the genimage.sh script, and that's relatively easy because the argument has a -c added to it. [snip] > [1] http://patchwork.ozlabs.org/patch/744825/ > [2] http://patchwork.ozlabs.org/patch/744826/ > [3] http://patchwork.ozlabs.org/patch/744824/
Arnout, On Tue, Apr 18, 2017 at 01:51:28PM +0200, Arnout Vandecappelle wrote: > [Adding Etienne to the Cc as the main contributor of genimage.sh] > > On 17-04-17 16:20, Gaël PORTAY wrote: > > Hello Abhimanyu, > > > [snip] > > Initially, genimage.sh script was NOT intend to be run either as or inside a > > post-image. It was a Makefile target [1]. [2] and [3] are use cases. > > True, but since [1] isn't applied yet and is still under discussion, we really > want to be able to call it as a post-image script directly. > Okay. The question is, do we "accept" to run genimage.sh inside a post-image script? If yes, developpers have to call genimage.sh with a fake first argument (or the binary directory). If no, the shift is a fair solution alongside a comment. A second solution consists in using this first argument in the script: # First argument is the binary directory bindir="$1" shift while getopts :c: OPT ; do case "${OPT}" in c) GENIMAGE_CFG="${OPTARG}";; :) die "option '${OPTARG}' expects a mandatory argument";; \?) die "unknown option '${OPTARG}'";; esac done # ... genimage \ --rootpath "${TARGET_DIR}" \ --tmppath "${GENIMAGE_TMP}" \ --inputpath "${bindir}" \ --outputpath "${bindir}" \ --config "${GENIMAGE_CFG}" But, if we deprecated the first argument, it also impacts the genimage.sh. > > > Maybe, a cleaner solution consists in updating the Makefile to remove this first > > argument given to both post-build and post-image scripts. But it breaks the > > existing. > > I don't think we can do that. Most "interesting" post-* scripts are out of > tree, so we can't change them. Although updating Buildroot is expected to have > some implications on the user integration layer (cfr. the change in br2-external > handling), we try to avoid it. > That is why I added Thomas and you in copy :) > It would indeed be good to remove that first argument, but then I think we have > to go through a (long) deprecation period. That means right now: mark it as > legacy in the manual and help text. > > > Thomas, Arnout, do you have a better idea? > > > > I had a quick look to scripts in-tree; they do not seem to use this parameter. > > > > Instead, they access directly to $TARGET_DIR or $BINARIES_DIR values using the > > environment variables. > > > > For extra arguments, they use $2, $3; they need to be updated. > > ... which proves that deprecating it is not easy, because it's fairly difficult > to make a script compatible with both the "new" and the "old" way - cfr. the > special care taking in the genimage.sh script, and that's relatively easy > because the argument has a -c added to it. I agree. > > [snip] > > [1] http://patchwork.ozlabs.org/patch/744825/ > > [2] http://patchwork.ozlabs.org/patch/744826/ > > [3] http://patchwork.ozlabs.org/patch/744824/ > Something that would be nice is to allow to give extra arguments to genimage. genimage.sh is acting as a wrapper. BR2_ROOTFS_POST_IMAGE_SCRIPT="support/scripts/genimage.sh" BR2_ROOTFS_POST_SCRIPT_ARGS="-c board/boardname/genimage.cfg -- --loglevel 1" Regards, Gaël
On 18-04-17 15:52, Gaël PORTAY wrote: > Arnout, > > On Tue, Apr 18, 2017 at 01:51:28PM +0200, Arnout Vandecappelle wrote: >> [Adding Etienne to the Cc as the main contributor of genimage.sh] >> >> On 17-04-17 16:20, Gaël PORTAY wrote: >>> Hello Abhimanyu, >>> >> [snip] >>> Initially, genimage.sh script was NOT intend to be run either as or inside a >>> post-image. It was a Makefile target [1]. [2] and [3] are use cases. >> >> True, but since [1] isn't applied yet and is still under discussion, we really >> want to be able to call it as a post-image script directly. >> > > Okay. > > The question is, do we "accept" to run genimage.sh inside a post-image script? Of course we do, because in some cases, genimage.sh cannot replace the post-image script - even with [1] applied, you still need a post-image script that calls genimage, and the genimage.sh wrapper is still useful in that case. > If yes, developpers have to call genimage.sh with a fake first argument (or the > binary directory). Well, Abhimanyu's patch avoids that, by ignoring non-option arguments. And this was the reason I told Etienne to use the -c option to begin with: to make handling of options more flexible. > > If no, the shift is a fair solution alongside a comment. > > A second solution consists in using this first argument in the script: > > # First argument is the binary directory > bindir="$1" > shift > while getopts :c: OPT ; do > case "${OPT}" in > c) GENIMAGE_CFG="${OPTARG}";; > :) die "option '${OPTARG}' expects a mandatory argument";; > \?) die "unknown option '${OPTARG}'";; > esac > done > > # ... > > genimage \ > --rootpath "${TARGET_DIR}" \ > --tmppath "${GENIMAGE_TMP}" \ > --inputpath "${bindir}" \ > --outputpath "${bindir}" \ > --config "${GENIMAGE_CFG}" > > But, if we deprecated the first argument, it also impacts the genimage.sh. Exactly. And it's not very useful, since it should *never* be different from BINARIES_DIR. >>> Maybe, a cleaner solution consists in updating the Makefile to remove this first >>> argument given to both post-build and post-image scripts. But it breaks the >>> existing. >> >> I don't think we can do that. Most "interesting" post-* scripts are out of >> tree, so we can't change them. Although updating Buildroot is expected to have >> some implications on the user integration layer (cfr. the change in br2-external >> handling), we try to avoid it. >> > > That is why I added Thomas and you in copy :) > >> It would indeed be good to remove that first argument, but then I think we have >> to go through a (long) deprecation period. That means right now: mark it as >> legacy in the manual and help text. >> >>> Thomas, Arnout, do you have a better idea? >>> >>> I had a quick look to scripts in-tree; they do not seem to use this parameter. >>> >>> Instead, they access directly to $TARGET_DIR or $BINARIES_DIR values using the >>> environment variables. >>> >>> For extra arguments, they use $2, $3; they need to be updated. >> >> ... which proves that deprecating it is not easy, because it's fairly difficult >> to make a script compatible with both the "new" and the "old" way - cfr. the >> special care taking in the genimage.sh script, and that's relatively easy >> because the argument has a -c added to it. > > I agree. > >> >> [snip] >>> [1] http://patchwork.ozlabs.org/patch/744825/ >>> [2] http://patchwork.ozlabs.org/patch/744826/ >>> [3] http://patchwork.ozlabs.org/patch/744824/ >> > > Something that would be nice is to allow to give extra arguments to genimage. > genimage.sh is acting as a wrapper. > > BR2_ROOTFS_POST_IMAGE_SCRIPT="support/scripts/genimage.sh" > BR2_ROOTFS_POST_SCRIPT_ARGS="-c board/boardname/genimage.cfg -- --loglevel 1" Yes, that could be useful. Although the -- makes is difficult again because it's not compatible with getopt so Abhimanyu's patch wouldn't work anymore. But I could accept it with extra quoting: BR2_ROOTFS_POST_SCRIPT_ARGS="-c board/boardname/genimage.cfg -O '--loglevel 1'" However, I don't really see the usefulness of this feature. loglevel is in fact the only option that you could usefully pass on to genimage. So just adding a -v option to genimage.sh is probably more convenient. Regards, Arnout
diff --git a/support/scripts/genimage.sh b/support/scripts/genimage.sh index 0ed0e8bcc..2b5549fb0 100755 --- a/support/scripts/genimage.sh +++ b/support/scripts/genimage.sh @@ -5,13 +5,18 @@ die() { exit 1 } +# Parse arguments and put into argument list of the script +eval set -- $(getopt -n genimage.sh -o c: -- "$@") + GENIMAGE_TMP="${BUILD_DIR}/genimage.tmp" -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";; +while true ; do + case "$1" in + -c) [ ! -z "$2" ] || die "option '${1}' expects a mandatory argument\n"; + GENIMAGE_CFG="${2}"; + shift 2 ;; + --) shift 1; break ;; + *) die "unknown option '${1}'\n";; esac done