[pacman-dev] [PATCH 1/8] scripts/library: introduce parseopts
This will replace our current options parser used in pacman-key and
makepkg. It follows heuristics closer to that of GNU getopt long (and
thus pacman itself), with the exception that it does not allow for
options with optional arguments. Due to the way this parser will be
used, this sort of functionality will not be needed.
Instead of relying on eval+set, options are normalized into an array,
OPTRET, which callers should expect to be populated after returning from
parseopts. This avoids problems with quotes and spaces in arguments,
assuming that the user quotes properly when passing into the
application.
A new test harness for parseopts is added in test/scripts.
Signed-off-by: Dave Reisner
Signed-off-by: Dave Reisner
On 13/04/12 00:54, Dave Reisner wrote:
Signed-off-by: Dave Reisner
Ack once a patch is added to prevent pkgname containing a comma. As no package in existence (being the Arch repos and the AUR...) has a comma in its name, that will not break anything so I decided it is best to be proactive here. Allan
Make this option additive, so that the following two operations are equivalent: makepkg --pkg foo --pkg bar makepkg --pkg foo,bar --- doc/makepkg.8.txt | 2 +- scripts/makepkg.sh.in | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/makepkg.8.txt b/doc/makepkg.8.txt index f80d7f2..57af98f 100644 --- a/doc/makepkg.8.txt +++ b/doc/makepkg.8.txt @@ -154,7 +154,7 @@ Options *\--pkg <list>*:: Only build listed packages from a split package. Multiple packages should - be comma separated in the list. + be comma separated in the list. This option can be specified multiple times. *\--check*:: Run the check() function in the PKGBUILD, overriding the setting in diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 4e3b0e3..d2510c1 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1997,7 +1997,7 @@ while true; do --nosign) SIGNPKG='n' ;; -o|--nobuild) NOBUILD=1 ;; -p) shift; BUILDFILE=$1 ;; - --pkg) shift; IFS=, read -ra PKGLIST <<<"$1" ;; + --pkg) shift; IFS=, read -ra p <<<"$1"; PKGLIST+=("${p[@]}"); unset p ;; -r|--rmdeps) RMDEPS=1 ;; -R|--repackage) REPKG=1 ;; --skipchecksums) SKIPCHECKSUMS=1 ;; -- 1.7.10
This requires an ugly amount of reworking of how pacman-key handles
options. The change simply to avoid passing keys, files, and directories
as arguments to options, but to leave them as arguments to the overall
program. This is reasonable since pacman-key limits the user to
essentially one operation per invocation (like pacman).
Since we now pass around the positional parameters to the various
operations, we can add some better sanity checking. Each operation is
responsible for testing input and making sure it can operate properly,
otherwise it throws an error and exits.
The doc is updated to reflect this, and uses similar verbiage as pacman,
describing the non-option arguments now passed to pacman-key as targets.
Similar to the doc, --help is reorganized to separate operations and
options and remove argument tokens from operations.
Signed-off-by: Dave Reisner
On 13/04/12 00:54, Dave Reisner wrote:
This requires an ugly amount of reworking of how pacman-key handles options. The change simply to avoid passing keys, files, and directories as arguments to options, but to leave them as arguments to the overall program. This is reasonable since pacman-key limits the user to essentially one operation per invocation (like pacman).
Since we now pass around the positional parameters to the various operations, we can add some better sanity checking. Each operation is responsible for testing input and making sure it can operate properly, otherwise it throws an error and exits.
The doc is updated to reflect this, and uses similar verbiage as pacman, describing the non-option arguments now passed to pacman-key as targets.
Similar to the doc, --help is reorganized to separate operations and options and remove argument tokens from operations.
Signed-off-by: Dave Reisner
--- I really hate this patch, but I don't think it makes sense to split it.
Agreed. One patch is fine. <snip>
+ if (( $# == 0 )); then + error "$(gettext "No keys specified")" + exit 1 + fi
This is repeated so many times... How about doing a single check below the check that only one operation is specified? <snip>
@@ -413,7 +428,7 @@ list_sigs() {
lsign_keys() { check_keyids_exist - printf 'y\ny\n' | LANG=C "${GPG_PACMAN[@]}" --command-fd 0 --quiet --batch --lsign-key "${KEYIDS[@]}" 2>/dev/null + printf '%s\n' y y | LANG=C "${GPG_PACMAN[@]}" --command-fd 0 --quiet --batch --lsign-key "$@" 2>/dev/null if (( PIPESTATUS[1] )); then error "$(gettext "A specified key could not be locally signed.")" exit 1
Is there a good reason beyond the cosmetic for the printf change? Because I think it fails if it is only cosmetic... Allan
On Thu, Apr 12, 2012 at 11:50 PM, Allan McRae
On 13/04/12 00:54, Dave Reisner wrote:
This requires an ugly amount of reworking of how pacman-key handles options. The change simply to avoid passing keys, files, and directories as arguments to options, but to leave them as arguments to the overall program. This is reasonable since pacman-key limits the user to essentially one operation per invocation (like pacman).
Since we now pass around the positional parameters to the various operations, we can add some better sanity checking. Each operation is responsible for testing input and making sure it can operate properly, otherwise it throws an error and exits.
The doc is updated to reflect this, and uses similar verbiage as pacman, describing the non-option arguments now passed to pacman-key as targets.
Similar to the doc, --help is reorganized to separate operations and options and remove argument tokens from operations.
Signed-off-by: Dave Reisner
--- I really hate this patch, but I don't think it makes sense to split it. Agreed. One patch is fine.
<snip>
+ if (( $# == 0 )); then + error "$(gettext "No keys specified")" + exit 1 + fi
This is repeated so many times... How about doing a single check below the check that only one operation is specified?
Because that would break any option that really can accept 0 arguments, such as --list-keys or --refresh-keys.
<snip>
@@ -413,7 +428,7 @@ list_sigs() {
lsign_keys() { check_keyids_exist - printf 'y\ny\n' | LANG=C "${GPG_PACMAN[@]}" --command-fd 0 --quiet --batch --lsign-key "${KEYIDS[@]}" 2>/dev/null + printf '%s\n' y y | LANG=C "${GPG_PACMAN[@]}" --command-fd 0 --quiet --batch --lsign-key "$@" 2>/dev/null if (( PIPESTATUS[1] )); then error "$(gettext "A specified key could not be locally signed.")" exit 1
Is there a good reason beyond the cosmetic for the printf change? Because I think it fails if it is only cosmetic...
It's only cosmetic, but I don't see how it fails... Anyways, it shouldn't be in the patch, so it's gone from my tree.
Allan
On 13/04/12 22:31, Dave Reisner wrote:
On Thu, Apr 12, 2012 at 11:50 PM, Allan McRae
wrote: On 13/04/12 00:54, Dave Reisner wrote:
This requires an ugly amount of reworking of how pacman-key handles options. The change simply to avoid passing keys, files, and directories as arguments to options, but to leave them as arguments to the overall program. This is reasonable since pacman-key limits the user to essentially one operation per invocation (like pacman).
Since we now pass around the positional parameters to the various operations, we can add some better sanity checking. Each operation is responsible for testing input and making sure it can operate properly, otherwise it throws an error and exits.
The doc is updated to reflect this, and uses similar verbiage as pacman, describing the non-option arguments now passed to pacman-key as targets.
Similar to the doc, --help is reorganized to separate operations and options and remove argument tokens from operations.
Signed-off-by: Dave Reisner
--- I really hate this patch, but I don't think it makes sense to split it. Agreed. One patch is fine.
<snip>
+ if (( $# == 0 )); then + error "$(gettext "No keys specified")" + exit 1 + fi
This is repeated so many times... How about doing a single check below the check that only one operation is specified?
Because that would break any option that really can accept 0 arguments, such as --list-keys or --refresh-keys.
Yeah. I meant something like: if (( (ADD || DELETE || EDITKEY || ....) && $# == 0 )); then
On Fri, Apr 13, 2012 at 8:39 AM, Allan McRae
On Thu, Apr 12, 2012 at 11:50 PM, Allan McRae
wrote: On 13/04/12 00:54, Dave Reisner wrote:
This requires an ugly amount of reworking of how pacman-key handles options. The change simply to avoid passing keys, files, and
as arguments to options, but to leave them as arguments to the overall program. This is reasonable since pacman-key limits the user to essentially one operation per invocation (like pacman).
Since we now pass around the positional parameters to the various operations, we can add some better sanity checking. Each operation is responsible for testing input and making sure it can operate properly, otherwise it throws an error and exits.
The doc is updated to reflect this, and uses similar verbiage as
On 13/04/12 22:31, Dave Reisner wrote: directories pacman,
describing the non-option arguments now passed to pacman-key as targets.
Similar to the doc, --help is reorganized to separate operations and options and remove argument tokens from operations.
Signed-off-by: Dave Reisner
--- I really hate this patch, but I don't think it makes sense to split it. Agreed. One patch is fine.
<snip>
+ if (( $# == 0 )); then + error "$(gettext "No keys specified")" + exit 1 + fi
This is repeated so many times... How about doing a single check below the check that only one operation is specified?
Because that would break any option that really can accept 0 arguments, such as --list-keys or --refresh-keys.
Yeah. I meant something like:
if (( (ADD || DELETE || EDITKEY || ....) && $# == 0 )); then
Ah, right. Yep, I can do that... looking back, seems I missed this in a few places anyways (import, verify, etc)
This is retired, as the two consumers of this function are now using the
new parseopts instead.
Signed-off-by: Dave Reisner
Loop through arguments passed to verify_sig and treat each as a
signature to be verified against a source file. Output each file as its
checked to avoid ambiguity.
Signed-off-by: Dave Reisner
On 13/04/12 00:54, Dave Reisner wrote:
Loop through arguments passed to verify_sig and treat each as a signature to be verified against a source file. Output each file as its checked to avoid ambiguity.
Signed-off-by: Dave Reisner
--- doc/pacman-key.8.txt | 2 +- scripts/pacman-key.sh.in | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/doc/pacman-key.8.txt b/doc/pacman-key.8.txt index 96ac31c..4a2122f 100644 --- a/doc/pacman-key.8.txt +++ b/doc/pacman-key.8.txt @@ -96,7 +96,7 @@ Operations Displays the program version.
*-v, \--verify*:: - Verify the given signature file. + Verify the given targets as signature files.
Not sure I like this wording... How about sticking with the wording in --help "Verify the file(s) specified by the signature(s)".
Options ------- diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in index b2c3da9..2083a60 100644 --- a/scripts/pacman-key.sh.in +++ b/scripts/pacman-key.sh.in @@ -66,7 +66,7 @@ usage() { printf -- "$(gettext " -l, --list-keys List the specified or all keys")\n" printf -- "$(gettext " -r, --recv-keys Fetch the specified keyids")\n" printf -- "$(gettext " -u, --updatedb Update the trustdb of pacman")\n" - printf -- "$(gettext " -v, --verify Verify the file specified by the signature")\n" + printf -- "$(gettext " -v, --verify Verify the file(s) specified by the signature(s)")\n" printf -- "$(gettext " --edit-key Present a menu for key management task on keyids")\n" printf -- "$(gettext " --import Imports pubring.gpg from dir(s)")\n" printf -- "$(gettext " --import-trustdb Imports ownertrust values from trustdb.gpg in dir(s)")\n" @@ -455,10 +455,15 @@ refresh_keys() { }
verify_sig() { - if ! "${GPG_PACMAN[@]}" --status-fd 1 --verify "$1" | grep -qE 'TRUST_(FULLY|ULTIMATE)'; then - error "$(gettext "The signature identified by %s could not be verified.")" "$1" - exit 1 - fi + local ret=0 + for sig; do + msg "Checking %s ..." "$sig" + if ! "${GPG_PACMAN[@]}" --status-fd 1 --verify "$sig" | grep -qE 'TRUST_(FULLY|ULTIMATE)'; then + error "$(gettext "The signature identified by %s could not be verified.")" "$sig" + ret=1 + fi + done + exit $ret }
updatedb() {
On Thu, Apr 12, 2012 at 11:25 PM, Allan McRae
On 13/04/12 00:54, Dave Reisner wrote:
Loop through arguments passed to verify_sig and treat each as a signature to be verified against a source file. Output each file as its checked to avoid ambiguity.
Signed-off-by: Dave Reisner
--- doc/pacman-key.8.txt | 2 +- scripts/pacman-key.sh.in | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/doc/pacman-key.8.txt b/doc/pacman-key.8.txt index 96ac31c..4a2122f 100644 --- a/doc/pacman-key.8.txt +++ b/doc/pacman-key.8.txt @@ -96,7 +96,7 @@ Operations Displays the program version.
*-v, \--verify*:: - Verify the given signature file. + Verify the given targets as signature files.
Not sure I like this wording... How about sticking with the wording in --help "Verify the file(s) specified by the signature(s)".
Agreed.
Options ------- diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in index b2c3da9..2083a60 100644 --- a/scripts/pacman-key.sh.in +++ b/scripts/pacman-key.sh.in @@ -66,7 +66,7 @@ usage() { printf -- "$(gettext " -l, --list-keys List the
specified or all keys")\n"
printf -- "$(gettext " -r, --recv-keys Fetch the
specified keyids")\n"
printf -- "$(gettext " -u, --updatedb Update the
- printf -- "$(gettext " -v, --verify Verify the file specified by the signature")\n" + printf -- "$(gettext " -v, --verify Verify the file(s) specified by the signature(s)")\n" printf -- "$(gettext " --edit-key Present a menu for key management task on keyids")\n" printf -- "$(gettext " --import Imports
trustdb of pacman")\n" pubring.gpg from dir(s)")\n"
printf -- "$(gettext " --import-trustdb Imports
ownertrust values from trustdb.gpg in dir(s)")\n"
@@ -455,10 +455,15 @@ refresh_keys() { }
verify_sig() { - if ! "${GPG_PACMAN[@]}" --status-fd 1 --verify "$1" | grep -qE 'TRUST_(FULLY|ULTIMATE)'; then - error "$(gettext "The signature identified by %s could not be verified.")" "$1" - exit 1 - fi + local ret=0 + for sig; do + msg "Checking %s ..." "$sig" + if ! "${GPG_PACMAN[@]}" --status-fd 1 --verify "$sig" | grep -qE 'TRUST_(FULLY|ULTIMATE)'; then + error "$(gettext "The signature identified by %s could not be verified.")" "$sig" + ret=1 + fi + done + exit $ret }
updatedb() {
Pass $(OURSCRIPTS) through the bash parser in read only mode to validate
syntax. Note that this doesn't actually catch all errors, but it might
be useful for developers working on these scripts.
Signed-off-by: Dave Reisner
On 13/04/12 00:54, Dave Reisner wrote:
Pass $(OURSCRIPTS) through the bash parser in read only mode to validate syntax. Note that this doesn't actually catch all errors, but it might be useful for developers working on these scripts.
Signed-off-by: Dave Reisner
--- contrib/Makefile.am | 1 + scripts/Makefile.am | 1 + 2 files changed, 2 insertions(+) diff --git a/contrib/Makefile.am b/contrib/Makefile.am index eca39e7..2953912 100644 --- a/contrib/Makefile.am +++ b/contrib/Makefile.am @@ -55,6 +55,7 @@ $(OURSCRIPTS): Makefile $(AM_V_GEN)$(edit) $(srcdir)/$@.in >$@.tmp $(AM_V_at)chmod +x,a-w $@.tmp $(AM_V_at)mv $@.tmp $@ + @$(BASH_SHELL) -O extglob -n $@
I'm missing why we need extglob here?
$(OURFILES): Makefile $(AM_V_at)$(RM) $@ $@.tmp diff --git a/scripts/Makefile.am b/scripts/Makefile.am index b8a1990..dbe61b8 100644 --- a/scripts/Makefile.am +++ b/scripts/Makefile.am @@ -63,6 +63,7 @@ edit = sed \ $(OURSCRIPTS): Makefile $(AM_V_at)$(RM) $@ $(AM_V_GEN)test -f $(srcdir)/$@.sh.in && m4 -P -I $(srcdir) $(srcdir)/$@.sh.in | $(edit) >$@ + @$(BASH_SHELL) -O extglob -n $@ $(AM_V_at)chmod +x,a-w $@
makepkg: \
On 13.04.2012 05:30, Allan McRae wrote:
On 13/04/12 00:54, Dave Reisner wrote:
Pass $(OURSCRIPTS) through the bash parser in read only mode to validate syntax. Note that this doesn't actually catch all errors, but it might be useful for developers working on these scripts.
Signed-off-by: Dave Reisner
--- contrib/Makefile.am | 1 + scripts/Makefile.am | 1 + 2 files changed, 2 insertions(+) diff --git a/contrib/Makefile.am b/contrib/Makefile.am index eca39e7..2953912 100644 --- a/contrib/Makefile.am +++ b/contrib/Makefile.am @@ -55,6 +55,7 @@ $(OURSCRIPTS): Makefile $(AM_V_GEN)$(edit) $(srcdir)/$@.in >$@.tmp $(AM_V_at)chmod +x,a-w $@.tmp $(AM_V_at)mv $@.tmp $@ + @$(BASH_SHELL) -O extglob -n $@
I'm missing why we need extglob here?
bacman and paccache use extglobs -- Florian Pritz
On 13/04/12 20:49, Florian Pritz wrote:
On 13.04.2012 05:30, Allan McRae wrote:
On 13/04/12 00:54, Dave Reisner wrote:
Pass $(OURSCRIPTS) through the bash parser in read only mode to validate syntax. Note that this doesn't actually catch all errors, but it might be useful for developers working on these scripts.
Signed-off-by: Dave Reisner
--- contrib/Makefile.am | 1 + scripts/Makefile.am | 1 + 2 files changed, 2 insertions(+) diff --git a/contrib/Makefile.am b/contrib/Makefile.am index eca39e7..2953912 100644 --- a/contrib/Makefile.am +++ b/contrib/Makefile.am @@ -55,6 +55,7 @@ $(OURSCRIPTS): Makefile $(AM_V_GEN)$(edit) $(srcdir)/$@.in >$@.tmp $(AM_V_at)chmod +x,a-w $@.tmp $(AM_V_at)mv $@.tmp $@ + @$(BASH_SHELL) -O extglob -n $@
I'm missing why we need extglob here?
bacman and paccache use extglobs
Yes... and every time someone calls one of those scripts they have to manually set the extglob? No... because that would be stupid.
On Fri, Apr 13, 2012 at 6:57 AM, Allan McRae
On 13/04/12 20:49, Florian Pritz wrote:
On 13.04.2012 05:30, Allan McRae wrote:
On 13/04/12 00:54, Dave Reisner wrote:
Pass $(OURSCRIPTS) through the bash parser in read only mode to validate syntax. Note that this doesn't actually catch all errors, but it might be useful for developers working on these scripts.
Signed-off-by: Dave Reisner
--- contrib/Makefile.am | 1 + scripts/Makefile.am | 1 + 2 files changed, 2 insertions(+) diff --git a/contrib/Makefile.am b/contrib/Makefile.am index eca39e7..2953912 100644 --- a/contrib/Makefile.am +++ b/contrib/Makefile.am @@ -55,6 +55,7 @@ $(OURSCRIPTS): Makefile $(AM_V_GEN)$(edit) $(srcdir)/$@.in >$@.tmp $(AM_V_at)chmod +x,a-w $@.tmp $(AM_V_at)mv $@.tmp $@ + @$(BASH_SHELL) -O extglob -n $@
I'm missing why we need extglob here?
bacman and paccache use extglobs
Yes... and every time someone calls one of those scripts they have to manually set the extglob? No... because that would be stupid.
It would be stupid, but this is bash... So let's ignore rational thought for a minute. $ bash -n ./scripts/repo-add ./scripts/repo-add: line 261: syntax error near unexpected token `(' ./scripts/repo-add: line 261: ` *.@(db|files).tar.gz) TAR_OPT="z" ;;' no-exec mode really does mean no-exec, including shopts that might change the behavior of the parser.
On 13/04/12 22:57, Dave Reisner wrote:
On Fri, Apr 13, 2012 at 6:57 AM, Allan McRae
wrote: On 13/04/12 20:49, Florian Pritz wrote:
On 13.04.2012 05:30, Allan McRae wrote:
On 13/04/12 00:54, Dave Reisner wrote:
Pass $(OURSCRIPTS) through the bash parser in read only mode to validate syntax. Note that this doesn't actually catch all errors, but it might be useful for developers working on these scripts.
Signed-off-by: Dave Reisner
--- contrib/Makefile.am | 1 + scripts/Makefile.am | 1 + 2 files changed, 2 insertions(+) diff --git a/contrib/Makefile.am b/contrib/Makefile.am index eca39e7..2953912 100644 --- a/contrib/Makefile.am +++ b/contrib/Makefile.am @@ -55,6 +55,7 @@ $(OURSCRIPTS): Makefile $(AM_V_GEN)$(edit) $(srcdir)/$@.in >$@.tmp $(AM_V_at)chmod +x,a-w $@.tmp $(AM_V_at)mv $@.tmp $@ + @$(BASH_SHELL) -O extglob -n $@
I'm missing why we need extglob here?
bacman and paccache use extglobs
Yes... and every time someone calls one of those scripts they have to manually set the extglob? No... because that would be stupid.
It would be stupid, but this is bash... So let's ignore rational thought for a minute.
$ bash -n ./scripts/repo-add ./scripts/repo-add: line 261: syntax error near unexpected token `(' ./scripts/repo-add: line 261: ` *.@(db|files).tar.gz) TAR_OPT="z" ;;'
no-exec mode really does mean no-exec, including shopts that might change the behavior of the parser.
Ok then. Bash does something I don't expect yet again! One thing that will do is cause any script where extglob is needed but not enabled to "pass". But I think fixing that would be a pain and also really is not necessary so ignore...
--- This was on the top of my parse-opts branch. Figured I'd send it along to see if anyone else cares about this sort of thing. Easily nuked... Since we're using associative arrays in pacman-key, 4.0 is a requirement. However, 4.0 is also full of bugs, so Allan and I agreed that 4.1 would be a better minimum requirement to avoid unresolvable bug reports. configure.ac | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index a6114f8..b325fbb 100644 --- a/configure.ac +++ b/configure.ac @@ -148,7 +148,15 @@ AC_PROG_AWK AC_PROG_CC_C99 AC_PROG_INSTALL AC_CHECK_PROGS([PYTHON], [python2.7 python2.6 python2.5 python2 python], [false]) -AC_PATH_PROGS([BASH_SHELL], [bash bash4 bash3], [false]) +AC_PATH_PROGS([BASH_SHELL], [bash bash4], [false]) + +AS_IF([test "x$BASH_SHELL" = "xfalse"], + AC_MSG_WARN([*** bash >= 4.1.0 is required for pacman scripts]), + [bash_version_majmin=`$BASH_SHELL -c 'echo "${BASH_VERSINFO[[0]]}${BASH_VERSINFO[[1]]}"'` + if test "$bash_version_majmin" -lt 41; then + AC_MSG_ERROR([*** bash >= 4.1.0 is required for pacman scripts]) + fi + unset bash_version_majmin]) # find installed gettext AM_GNU_GETTEXT([external], [need-ngettext]) -- 1.7.10
On 13/04/12 00:54, Dave Reisner wrote:
--- This was on the top of my parse-opts branch. Figured I'd send it along to see if anyone else cares about this sort of thing. Easily nuked...
Since we're using associative arrays in pacman-key, 4.0 is a requirement. However, 4.0 is also full of bugs, so Allan and I agreed that 4.1 would be a better minimum requirement to avoid unresolvable bug reports.
I agree that we agreed that 4.1 is the way to go. Bash-4.1 was released in Jan 2010.
configure.ac | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac index a6114f8..b325fbb 100644 --- a/configure.ac +++ b/configure.ac @@ -148,7 +148,15 @@ AC_PROG_AWK AC_PROG_CC_C99 AC_PROG_INSTALL AC_CHECK_PROGS([PYTHON], [python2.7 python2.6 python2.5 python2 python], [false]) -AC_PATH_PROGS([BASH_SHELL], [bash bash4 bash3], [false]) +AC_PATH_PROGS([BASH_SHELL], [bash bash4], [false]) + +AS_IF([test "x$BASH_SHELL" = "xfalse"], + AC_MSG_WARN([*** bash >= 4.1.0 is required for pacman scripts]), + [bash_version_majmin=`$BASH_SHELL -c 'echo "${BASH_VERSINFO[[0]]}${BASH_VERSINFO[[1]]}"'` + if test "$bash_version_majmin" -lt 41; then + AC_MSG_ERROR([*** bash >= 4.1.0 is required for pacman scripts]) + fi + unset bash_version_majmin])
# find installed gettext AM_GNU_GETTEXT([external], [need-ngettext])
On 13/04/12 00:54, Dave Reisner wrote:
This will replace our current options parser used in pacman-key and makepkg. It follows heuristics closer to that of GNU getopt long (and thus pacman itself), with the exception that it does not allow for options with optional arguments. Due to the way this parser will be used, this sort of functionality will not be needed.
Instead of relying on eval+set, options are normalized into an array, OPTRET, which callers should expect to be populated after returning from parseopts. This avoids problems with quotes and spaces in arguments, assuming that the user quotes properly when passing into the application.
A new test harness for parseopts is added in test/scripts.
Signed-off-by: Dave Reisner
---
<snip>
+ + get_argreq() { + local o re="^($1)(:?:?)$"
You only want one :? in the regex.
+ for o in "${longopts[@]}"; do + # found option, return number of colons (0 = none, 1 = required) + [[ $o =~ $re ]] && return ${#BASH_REMATCH[2]} + done + # failure + return 255 + }
<snip>
+test_result() { + local result=$1 tokencount=$2 input=$3; shift 3 + + if { [[ $result = "$*" ]] || [[ $2 = NULL && -z $1 ]]; } && (( tokencount == $# )); then
What is the "|| [[ $2 = NULL && -z $1 ]];" part for? I can guess, but it seems unneeded. With the minor changes I pointed out in the last few emails, I ack this patchset. My biggest complaint is I prefer the name "parse_options" over "parseopts"! Allan
On Fri, Apr 13, 2012 at 12:46 AM, Allan McRae
On 13/04/12 00:54, Dave Reisner wrote:
This will replace our current options parser used in pacman-key and makepkg. It follows heuristics closer to that of GNU getopt long (and thus pacman itself), with the exception that it does not allow for options with optional arguments. Due to the way this parser will be used, this sort of functionality will not be needed.
Instead of relying on eval+set, options are normalized into an array, OPTRET, which callers should expect to be populated after returning from parseopts. This avoids problems with quotes and spaces in arguments, assuming that the user quotes properly when passing into the application.
A new test harness for parseopts is added in test/scripts.
Signed-off-by: Dave Reisner
--- <snip>
+ + get_argreq() { + local o re="^($1)(:?:?)$"
You only want one :? in the regex.
Ah yes.. a vestigial from when I was toying with optional arguments.
+ for o in "${longopts[@]}"; do + # found option, return number of colons (0 = none, 1 = required) + [[ $o =~ $re ]] && return ${#BASH_REMATCH[2]} + done + # failure + return 255 + }
<snip>
+test_result() { + local result=$1 tokencount=$2 input=$3; shift 3 + + if { [[ $result = "$*" ]] || [[ $2 = NULL && -z $1 ]]; } && (( tokencount == $# )); then
What is the "|| [[ $2 = NULL && -z $1 ]];" part for? I can guess, but it seems unneeded.
I have no idea what it's for. This test toy (and some of the parseopts code) dates back to July-ish of last year. Removing the entire middle check seems innocuous, so its gone.
With the minor changes I pointed out in the last few emails, I ack this patchset. My biggest complaint is I prefer the name "parse_options" over "parseopts"!
Allan
participants (4)
-
Allan McRae
-
Dave Reisner
-
Dave Reisner
-
Florian Pritz