Re: [PATCH 2/4] easy-build.sh: use getopts instead of rolling our own option parsing.
On Friday 07 August 2009, Ian Campbell wrote: > # Set configuration file to be used for the build and source it > -export CF=CONF.sh > +export CF=./CONF.sh I've already committed that (for all scripts that set/source it). -- To UNSUBSCRIBE, email to debian-cd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Re: [PATCH 2/4] easy-build.sh: use getopts instead of rolling our own option parsing.
Hi, one nitpick: On Fri, Aug 7, 2009 at 13:33:33 +0100, Ian Campbell wrote: > +shift $(expr $OPTIND - 1) > maybe 'shift $((OPTIND - 1))' so you don't fork expr? Cheers, Julien -- To UNSUBSCRIBE, email to debian-cd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Re: [PATCH 2/4] easy-build.sh: use getopts instead of rolling our own option parsing.
Ian Campbell wrote: On Fri, 2009-08-07 at 13:33 +0100, Ian Campbell wrote: I just tried it and it looks like the script is already not dash-ready: $ dash -x ./easy-build.sh -h + set -e + export CF=CONF.sh + . CONF.sh .: 1: CONF.sh: not found I'm no expert but it looks like dash obeys $PATH when executing the ".". The manpage doesn't mention this behaviour and bash doesn't seem to follow it but "." is pretty hard to google for so I have looked up the docs... dot should look after PATH, see POSIX: http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#dot so dash is correcter. ciao cate -- To UNSUBSCRIBE, email to debian-cd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Re: [PATCH 2/4] easy-build.sh: use getopts instead of rolling our own option parsing.
On Fri, 2009-08-07 at 13:45 +0100, Ian Campbell wrote:
> On Fri, 2009-08-07 at 13:33 +0100, Ian Campbell wrote:
> >
> > I just tried it and it looks like the script is already not
> > dash-ready:
> > $ dash -x ./easy-build.sh -h
> > + set -e
> > + export CF=CONF.sh
> > + . CONF.sh
> > .: 1: CONF.sh: not found
>
> I'm no expert but it looks like dash obeys $PATH when executing the ".".
> The manpage doesn't mention this behaviour and bash doesn't seem to
> follow it but "." is pretty hard to google for so I have looked up the
haven't
> docs...
>
> $ PATH=.:$PATH ./easy-build.sh -h
> Usage: easy-build.sh [-h] [-d gnome|kde|lxde|xfce|light|all]
> BC|NETINST|CD|DVD [ ...]
>
> works for me. So does the following I've no idea if this is correct
> analysis though...
>
> --- a/easy-build.sh
> +++ b/easy-build.sh
> @@ -11,7 +11,7 @@ show_usage() {
>
>
> # Set configuration file to be used for the build and source it
> -export CF=CONF.sh
> +export CF=./CONF.sh
> . $CF
> export DEBIAN_CD_CONF_SOURCED=true
> unset UPDATE_LOCAL
>
> --
> Ian Campbell
> Current Noise: Black Label Society - Bleed For Me
>
> First law of debate:
> Never argue with a fool. People might not know the difference.
>
>
--
Ian Campbell
Current Noise: Black Label Society - Lords Of Destruction
In Brooklyn, we had such great pennant races, it made the World Series
just something that came later.
-- Walter O'Malley, Dodgers owner
--
To UNSUBSCRIBE, email to debian-cd-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Re: [PATCH 2/4] easy-build.sh: use getopts instead of rolling our own option parsing.
On Fri, 2009-08-07 at 13:33 +0100, Ian Campbell wrote:
>
> I just tried it and it looks like the script is already not
> dash-ready:
> $ dash -x ./easy-build.sh -h
> + set -e
> + export CF=CONF.sh
> + . CONF.sh
> .: 1: CONF.sh: not found
I'm no expert but it looks like dash obeys $PATH when executing the ".".
The manpage doesn't mention this behaviour and bash doesn't seem to
follow it but "." is pretty hard to google for so I have looked up the
docs...
$ PATH=.:$PATH ./easy-build.sh -h
Usage: easy-build.sh [-h] [-d gnome|kde|lxde|xfce|light|all]
BC|NETINST|CD|DVD [ ...]
works for me. So does the following I've no idea if this is correct
analysis though...
--- a/easy-build.sh
+++ b/easy-build.sh
@@ -11,7 +11,7 @@ show_usage() {
# Set configuration file to be used for the build and source it
-export CF=CONF.sh
+export CF=./CONF.sh
. $CF
export DEBIAN_CD_CONF_SOURCED=true
unset UPDATE_LOCAL
--
Ian Campbell
Current Noise: Black Label Society - Bleed For Me
First law of debate:
Never argue with a fool. People might not know the difference.
--
To UNSUBSCRIBE, email to debian-cd-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Re: [PATCH 2/4] easy-build.sh: use getopts instead of rolling our own option parsing.
On Fri, 2009-08-07 at 14:05 +0200, Frans Pop wrote:
> On Friday 07 August 2009, Ian Campbell wrote:
> > +while getopts d:h OPT ; do
>
> Is getopts also supported in dash?
I believe so.
I just tried it and it looks like the script is already not dash-ready:
$ dash -x ./easy-build.sh -h
+ set -e
+ export CF=CONF.sh
+ . CONF.sh
.: 1: CONF.sh: not found
(Just to show dash knows about getops since the above didn't get that
far:
$ dash -c 'getopts'
getopts: 1: Usage: getopts optstring var [arg]
)
> > + case $OPT in
> > + d)
> > + case $OPTARG in
> > + # Note: "gnome" is the special gnome task, not the generic
> > task
> > + gnome|kde|lxde|xfce|light|all)
> > + desktop=$2
> > + ;;
> > + *)
> > + show_usage
> > + exit 1
> > + ;;
> > + esac ;;
> > + h) show_usage; exit 1;;
>
> Please put the commands for the "h" option on separate lines (as is done
> for the others).
Done.
> AFAIK a plain 'show_usage', if not displayed as the result of an error,
> should have an 'exit 0'.
Done.
> The -h option should be listed in the usage output.
Done.
> Maybe it would be good to also support --help if -h is added.
Since getopts doesn't support long options I agree with Max that it
isn't worth the complexity.
Updated patch below.
Ian.
easy-build.sh: use getopts instead of rolling our own option parsing.
---
easy-build.sh | 29 ++---
1 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/easy-build.sh b/easy-build.sh
index c586c1e..5e7a31a 100755
--- a/easy-build.sh
+++ b/easy-build.sh
@@ -6,7 +6,7 @@ set -e
## See also CONF.sh for the meaning of variables used here.
show_usage() {
- echo "Usage: $(basename $0) [-d gnome|kde|lxde|xfce|light|all]
BC|NETINST|CD|DVD [ ...]"
+ echo "Usage: $(basename $0) [-h] [-d gnome|kde|lxde|xfce|light|all]
BC|NETINST|CD|DVD [ ...]"
}
@@ -25,19 +25,26 @@ if [ $# -eq 0 ]; then
fi
desktop=
-if [ "$1" = "-d" ]; then
- case $2 in
- # Note: "gnome" is the special gnome task, not the generic task
- gnome|kde|lxde|xfce|light|all)
- desktop=$2
- shift 2
- ;;
- *)
+while getopts d:h OPT ; do
+ case $OPT in
+ d)
+ case $OPTARG in
+ # Note: "gnome" is the special gnome task, not the generic task
+ gnome|kde|lxde|xfce|light|all)
+ desktop=$2
+ ;;
+ *)
+ show_usage
+ exit 1
+ ;;
+ esac ;;
+ h)
show_usage
- exit 1
+ exit 0
;;
esac
-fi
+done
+shift $(expr $OPTIND - 1)
export DISKTYPE="$1"
shift
--
1.6.3.3
--
Ian Campbell
Current Noise: Witchcraft - It's So Easy
Logic is the chastity belt of the mind!
--
To UNSUBSCRIBE, email to debian-cd-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Re: [PATCH 2/4] easy-build.sh: use getopts instead of rolling our own option parsing.
On Fri, Aug 07, 2009 at 02:05:52PM +0200, Frans Pop wrote: > On Friday 07 August 2009, Ian Campbell wrote: > > +while getopts d:h OPT ; do > > Is getopts also supported in dash? Yes, and in POSIX. > Maybe it would be good to also support --help if -h is added. getopts can't do longoptions. I'd say supporting --help is probably not worth the extra code. Max -- To UNSUBSCRIBE, email to debian-cd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Re: [PATCH 2/4] easy-build.sh: use getopts instead of rolling our own option parsing.
On Friday 07 August 2009, Ian Campbell wrote: > +while getopts d:h OPT ; do Is getopts also supported in dash? > + case $OPT in > + d) > + case $OPTARG in > + # Note: "gnome" is the special gnome task, not the generic > task > + gnome|kde|lxde|xfce|light|all) > + desktop=$2 > + ;; > + *) > + show_usage > + exit 1 > + ;; > + esac ;; > + h) show_usage; exit 1;; Please put the commands for the "h" option on separate lines (as is done for the others). AFAIK a plain 'show_usage', if not displayed as the result of an error, should have an 'exit 0'. The -h option should be listed in the usage output. Maybe it would be good to also support --help if -h is added.
