[pacman-dev] bash-3 only error trap activation
Hi, When running makepkg in bash-3.2 I am getting a couple of error trap activations that we do not get in bash-4.1. Here is the problem area: check_deps() { (( $# > 0 )) || return <- HERE local ret=0 pmout=$(run_pacman -T "$@") <- HERE ret=$? The first one gets set off anytime "depends" or "makedepends" are empty and can be fixed by using "|| return 0", but the second is doing my head in... Of course, turning off the error trap around those commands makes it work, and that may be an OK approach given we are dealing with all error states below that. If you want a bash-3.2 package to test grab one from http://allanmcrae.com/packages/bash3-3.2.051-1.src.tar.gz . Test on a PKGBUILD with unsatisfied dependencies. Allan
On Mon, Jun 21, 2010 at 1:06 AM, Allan McRae <allan@archlinux.org> wrote:
pmout=$(run_pacman -T "$@") <- HERE ret=$?
The first one gets set off anytime "depends" or "makedepends" are empty and can be fixed by using "|| return 0", but the second is doing my head in... Of course, turning off the error trap around those commands makes it work, and that may be an OK approach given we are dealing with all error states below that.
Try: ret=$? pmout=$(run_pacman -T "$@") || ret=$? Using || after the asigment will prevent setting the err trap. Funny how you noticed the first, because I was about to submit a patch that did not return false if there were no arguments. In reality though, it should be this: [[ $@ ]] || return 0 Because (( $# )) will not count emtpy arguments. If check_deps is passed quoted arguments that expand to nothing, it will malfunction. Andres P
On Mon, Jun 21, 2010 at 1:12 PM, Andres P <aepd87@gmail.com> wrote:
Try:
ret=$?
Sorry, ret=0 Andres P
On 22/06/10 03:42, Andres P wrote:
On Mon, Jun 21, 2010 at 1:06 AM, Allan McRae<allan@archlinux.org> wrote:
pmout=$(run_pacman -T "$@")<- HERE ret=$?
The first one gets set off anytime "depends" or "makedepends" are empty and can be fixed by using "|| return 0", but the second is doing my head in... Of course, turning off the error trap around those commands makes it work, and that may be an OK approach given we are dealing with all error states below that.
Try:
ret=$? pmout=$(run_pacman -T "$@") || ret=$?
Using || after the asigment will prevent setting the err trap.
Of course I tried that... It does not work: ==> Making package: pacman-contrib 3.4.0-1 (Tue Jun 22 08:46:58 EST 2010) ==> Checking Runtime Dependencies... ==> 127 ($ret value from within run_pacman) ==> ERROR: An unknown error has occurred. Exiting... ==> 1 ($ret value after "|| ret=$?") ==> ERROR: 'pacman' returned a fatal error (1): pacman>4 (wrong return value so wrong error message) I looks like in bash-3.2 the following happens. run_pacman returns 127 which sets off the error trap. Then the assignment fails which sets of another error trap. The use of "|| ret=$?" prevents the assignment failure error but now there is the wrong return value.
Funny how you noticed the first, because I was about to submit a patch that did not return false if there were no arguments.
In reality though, it should be this: [[ $@ ]] || return 0
Because (( $# )) will not count emtpy arguments.
If check_deps is passed quoted arguments that expand to nothing, it will malfunction.
I think that if check_deps is passed empty arguments, then there is not dependencies to check and it should just return. Allan
On Mon, Jun 21, 2010 at 7:33 PM, Allan McRae <allan@archlinux.org> wrote:
Of course I tried that... It does not work:
==> Making package: pacman-contrib 3.4.0-1 (Tue Jun 22 08:46:58 EST 2010) ==> Checking Runtime Dependencies... ==> 127 ($ret value from within run_pacman) ==> ERROR: An unknown error has occurred. Exiting... ==> 1 ($ret value after "|| ret=$?") ==> ERROR: 'pacman' returned a fatal error (1): pacman>4 (wrong return value so wrong error message)
I looks like in bash-3.2 the following happens. run_pacman returns 127 which sets off the error trap. Then the assignment fails which sets of another error trap. The use of "|| ret=$?" prevents the assignment failure error but now there is the wrong return value.
This is not what anybody wants to hear, but currently the ERR trap that everything inherits thanks to set -E is just a place holder for a traceback. Echoing "unrecognized error" isn't helpful at all, and if this is one of the items that is holding back bash 3.2 compat, then its inclusion should be reconsidered. Since trap_exit's symlink cleanup is actually useful, it would be merged to trap 0, aka clean_up, because it triggers on every signal except kill -9. This would also shorten other trap definitions.
Funny how you noticed the first, because I was about to submit a patch that did not return false if there were no arguments.
In reality though, it should be this: [[ $@ ]] || return 0
Because (( $# )) will not count emtpy arguments.
If check_deps is passed quoted arguments that expand to nothing, it will malfunction.
I think that if check_deps is passed empty arguments, then there is not dependencies to check and it should just return.
Currently a coincidence with no design consideration; deplist is unquoted and depends is handled like a unquoted array (see line 1892). What's the point of using arrays and not quoting them? If arrays get treated as scalars, then there's no telling if at any point a certain var needs special consideration or not. If deplist becomes an array, as it should since there is no reason to not quote depends and makedepends, then check_deps needs a string check via [[ $@ ]]. Basically, anything that can be quoted, should be. That way you don't have to hear about bugs with depends arrays with spaces and other special shell characters, like redirection operators, 6 months from now. Remember $startdir/4? This could have been prevented with quoting out of practice, specially for su since its arguments are parsed just like eval's are. And depends is where you get '>' and '<'... Andres P
Ok, after actually taking the time to install bash3... $ env -i HOME="$HOME" TERM="$TERM" bash3 <<\! set -o errexit set -o errtrace TRIGGERED_ERR() { return $?; } trap 'TRIGGERED_ERR' ERR set -o xtrace var=$(false) || true echo $? var=$(false || true) # only way of not triggering it... echo $? ! ++ false # Subshell false +++ TRIGGERED_ERR # Ignores outer "|| true" +++ return 1 + var= + true + echo 0 0 # But the entire command line does # not set off errexit ++ false ++ true # Predictable second subshell... + var= + echo 0 0 So, if you want to keep the ERR trap then you'll have to modify check_deps so that it always returns true. Then you'd have to parse its output, similar to how in_opt_array works. Horrible kludge, lets drop set -E. Andres P
On 22/06/10 14:23, Andres P wrote:
Ok, after actually taking the time to install bash3...
$ env -i HOME="$HOME" TERM="$TERM" bash3<<\!
set -o errexit set -o errtrace
TRIGGERED_ERR() { return $?; }
trap 'TRIGGERED_ERR' ERR
set -o xtrace
var=$(false) || true echo $?
var=$(false || true) # only way of not triggering it... echo $?
!
++ false # Subshell false +++ TRIGGERED_ERR # Ignores outer "|| true" +++ return 1 + var= + true + echo 0 0 # But the entire command line does # not set off errexit ++ false ++ true # Predictable second subshell... + var= + echo 0 0
So, if you want to keep the ERR trap then you'll have to modify check_deps so that it always returns true.
Then you'd have to parse its output, similar to how in_opt_array works.
Horrible kludge, lets drop set -E.
I do not think that dropping 'set -E' completely is the way to go. Just dropping it around that pacman call is enough. This is the diff I am proposing: @@ -382,11 +382,15 @@ } check_deps() { - (( $# > 0 )) || return + (( $# > 0 )) || return 0 + # Disable error trap in pacman subshell call as this breaks bash-3.2 compatibility + # Also, a non-zero return value is not unexpected and we are manually dealing them + set +E local ret=0 - pmout=$(run_pacman -T "$@") - ret=$? + pmout=$(run_pacman -T "$@") || ret=$? + set -E + if (( ret == 127 )); then #unresolved deps echo "$pmout" elif (( ret )); then Allan
On Tue, Jun 22, 2010 at 12:30 AM, Allan McRae <allan@archlinux.org> wrote:
I do not think that dropping 'set -E' completely is the way to go. Just dropping it around that pacman call is enough.
Another candidate for the style guide since it's obfuscated enough. btw makepkg is also skirting around -E when it sources /etc/profile...
This is the diff I am proposing:
@@ -382,11 +382,15 @@ }
check_deps() { - (( $# > 0 )) || return + (( $# > 0 )) || return 0
+ # Disable error trap in pacman subshell call as this breaks bash-3.2 compatibility + # Also, a non-zero return value is not unexpected and we are manually dealing them + set +E local ret=0 - pmout=$(run_pacman -T "$@") - ret=$? + pmout=$(run_pacman -T "$@") || ret=$? + set -E + if (( ret == 127 )); then #unresolved deps echo "$pmout" elif (( ret )); then
Let me know what repo is this diff going to so I can rebase 'undeclared local vars' and 'do not ignore pacman errors' against it. Andres P
participants (2)
-
Allan McRae
-
Andres P