[pacman-dev] [PATCH] scripts: replace test builtin [ with shell keywords [[ and ((
FS#16623 suggested this change for makepkg; this patch applies it to the remaining files in the scripts directory. Signed-off-by: Cedric Staniewski <cedric@gmx.ca> --- scripts/pacman-optimize.sh.in | 22 ++++++------ scripts/pkgdelta.sh.in | 22 ++++++------ scripts/rankmirrors.sh.in | 12 +++--- scripts/repo-add.sh.in | 74 ++++++++++++++++++++-------------------- 4 files changed, 65 insertions(+), 65 deletions(-) diff --git a/scripts/pacman-optimize.sh.in b/scripts/pacman-optimize.sh.in index e43e946..d9f1ca2 100644 --- a/scripts/pacman-optimize.sh.in +++ b/scripts/pacman-optimize.sh.in @@ -74,23 +74,23 @@ die_r() { # PROGRAM START # determine whether we have gettext; make it a no-op if we do not -if [ ! $(type -t gettext) ]; then +if ! type gettext &>/dev/null; then gettext() { echo "$@" } fi -if [ "$1" = "-h" -o "$1" = "--help" ]; then +if [[ $1 = "-h" || $1 = "--help" ]]; then usage exit 0 fi -if [ "$1" = "-V" -o "$1" = "--version" ]; then +if [[ $1 = "-V" || $1 = "--version" ]]; then version exit 0 fi -if [ "$1" != "" ]; then +if [[ -n $1 ]]; then dbroot="$1" fi @@ -99,11 +99,11 @@ if ! type diff >/dev/null 2>&1; then die "$(gettext "diff tool was not found, please install diffutils.")" fi -if [ ! -d "$dbroot" ]; then +if [[ ! -d $dbroot ]]; then die "$(gettext "%s does not exist or is not a directory.")" "$dbroot" fi -if [ ! -w "$dbroot" ]; then +if [[ ! -w $dbroot ]]; then die "$(gettext "You must have correct permissions to optimize the database.")" fi @@ -113,7 +113,7 @@ dbroot="${dbroot%/}" lockfile="${dbroot}/db.lck" # make sure pacman isn't running -if [ -f "$lockfile" ]; then +if [[ -f $lockfile ]]; then die "$(gettext "Pacman lock file was found. Cannot run while pacman is running.")" fi # do not let pacman run while we do this @@ -130,7 +130,7 @@ find "$dbroot" -type f | sort | xargs md5sum > "$workdir/pacsums.old" msg "$(gettext "Tar'ing up %s...")" "$dbroot" cd "$dbroot" bsdtar -czf "$workdir/pacman-db.tar.gz" ./ -if [ $? -ne 0 ]; then +if (( $? )); then rm -rf "$workdir" die_r "$(gettext "Tar'ing up %s failed.")" "$dbroot" fi @@ -139,7 +139,7 @@ fi msg "$(gettext "Making and MD5sum'ing the new database...")" mkdir "$dbroot.new" bsdtar -xpf "$workdir/pacman-db.tar.gz" -C "$dbroot.new" -if [ $? -ne 0 ]; then +if (( $? )); then rm -rf "$workdir" die_r "$(gettext "Untar'ing %s failed.")" "$dbroot" fi @@ -152,7 +152,7 @@ find "$dbroot.new" -type f | sort | \ # step 4: compare the sums msg "$(gettext "Checking integrity...")" diff "$workdir/pacsums.old" "$workdir/pacsums.new" >/dev/null 2>&1 -if [ $? -ne 0 ]; then +if (( $? )); then # failed # leave our pacman-optimize tmpdir for checking to see what doesn't match up rm -rf "$dbroot.new" @@ -167,7 +167,7 @@ mv "$dbroot" "$dbroot.old" || fail=1 mv "$dbroot.new" "$dbroot" || fail=1 chmod --reference="$dbroot.old" "$dbroot" || fail=1 chown --reference="$dbroot.old" "$dbroot" || fail=1 -if [ $fail -ne 0 ]; then +if (( fail )); then # failure with our directory shuffle die_r "$(gettext "New database substitution failed. Check for $dbroot,\n$dbroot.old, and $dbroot.new directories.")" fi diff --git a/scripts/pkgdelta.sh.in b/scripts/pkgdelta.sh.in index 588dc49..1550fa1 100644 --- a/scripts/pkgdelta.sh.in +++ b/scripts/pkgdelta.sh.in @@ -35,7 +35,7 @@ QUIET=0 umask 0022 msg() { - [ $QUIET -ne 0 ] && return + (( QUIET )) && return local mesg=$1; shift printf "==> ${mesg}\n" "$@" >&1 } @@ -79,7 +79,7 @@ read_pkginfo() for line in $(bsdtar -xOf "$1" .PKGINFO 2>/dev/null | grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1="\2"|'); do eval "$line" - if [ -n "$pkgname" -a -n "$pkgver" -a -n "$arch" ]; then + if [[ -n $pkgname && -n $pkgver && -n $arch ]]; then IFS=$OLDIFS return 0 fi @@ -108,17 +108,17 @@ create_xdelta() newver="$pkgver" newarch="$arch" - if [ "$oldname" != "$newname" ]; then + if [[ $oldname != $newname ]]; then error "$(gettext "The package names don't match : '%s' and '%s'")" "$oldname" "$newname" return 1 fi - if [ "$oldarch" != "$newarch" ]; then + if [[ $oldarch != $newarch ]]; then error "$(gettext "The package architectures don't match : '%s' and '%s'")" "$oldarch" "$newarch" return 1 fi - if [ "$oldver" == "$newver" ]; then + if [[ $oldver == $newver ]]; then error "$(gettext "Both packages have the same version : '%s'")" "$newver" return 1 fi @@ -128,12 +128,12 @@ create_xdelta() local ret=0 xdelta3 -q -f -s "$oldfile" "$newfile" "$deltafile" || ret=$? - if [ $ret -ne 0 ]; then + if (( ret )); then error "$(gettext "Delta could not be created.")" return 1 else msg "$(gettext "Generated delta : '%s'")" "$deltafile" - [ $QUIET -eq 1 ] && echo "$deltafile" + (( QUIET )) && echo "$deltafile" fi return 0 } @@ -142,22 +142,22 @@ case "$1" in -q|--quiet) QUIET=1; shift ;; esac -if [ $# -ne 2 ]; then +if (( $# != 2 )); then usage exit 0 fi -if [ ! -f "$1" ]; then +if [[ ! -f $1 ]]; then error "$(gettext "File '%s' does not exist")" "$1" exit 0 fi -if [ ! -f "$2" ]; then +if [[ ! -f $2 ]]; then error "$(gettext "File '%s' does not exist")" "$2" exit 0 fi -if [ ! "$(type -p xdelta3)" ]; then +if ! type xdelta3 &>/dev/null; then error "$(gettext "Cannot find the xdelta3 binary! Is xdelta3 installed?")" exit 1 fi diff --git a/scripts/rankmirrors.sh.in b/scripts/rankmirrors.sh.in index 015b39a..d5cbadd 100644 --- a/scripts/rankmirrors.sh.in +++ b/scripts/rankmirrors.sh.in @@ -56,8 +56,8 @@ err() { # returns the fetching time, or timeout, or unreachable gettime() { IFS=' ' output=( $(curl -s -m 10 -w "%{time_total} %{http_code}" "$1" -o/dev/null) ) - [[ $? = 28 ]] && echo timeout && return - [[ ${output[1]} -ge 400 || ${output[1]} -eq 0 ]] && echo unreachable && return + (( $? == 28 )) && echo timeout && return + (( ${output[1]} >= 400 || ! ${output[1]} )) && echo unreachable && return echo "${output[0]}" } @@ -96,13 +96,13 @@ finaloutput() { for line in "${sortedarray[@]}"; do echo "${line#* } : ${line% *}" ((numiterator++)) - [[ $NUM -ne 0 && $numiterator -ge $NUM ]] && break + (( NUM && numiterator >= NUM )) && break done else for line in "${sortedarray[@]}"; do echo "Server = ${line#* }" ((numiterator++)) - [[ $NUM -ne 0 && $numiterator -ge $NUM ]] && break + (( NUM && numiterator >= NUM )) && break done fi exit 0 @@ -112,7 +112,7 @@ finaloutput() { # Argument parsing [[ $1 ]] || usage while [[ $1 ]]; do - if [[ "${1:0:2}" = -- ]]; then + if [[ ${1:0:2} = -- ]]; then case "${1:2}" in help) usage ;; version) version ;; @@ -142,7 +142,7 @@ while [[ $1 ]]; do done shift $snum fi - elif [[ -f "$1" ]]; then + elif [[ -f $1 ]]; then FILE="1" IFS=$'\n' linearray=( $(<$1) ) [[ $linearray ]] || err "File is empty." diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 3f00441..0e46c37 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -42,7 +42,7 @@ msg() { } msg2() { - [ $QUIET -ne 0 ] && return + (( QUIET )) && return local mesg=$1; shift printf " -> ${mesg}\n" "$@" >&1 } @@ -90,7 +90,7 @@ There is NO WARRANTY, to the extent permitted by law.\n")" # arg2 - List # arg3 - File to write to write_list_entry() { - if [ -n "$2" ]; then + if [[ -n $2 ]]; then echo "%$1%" >>$3 echo -e $2 >>$3 fi @@ -102,7 +102,7 @@ find_pkgentry() local pkgentry for pkgentry in $tmpdir/$pkgname*; do name=${pkgentry##*/} - if [ "${name%-*-*}" = "$pkgname" ]; then + if [[ ${name%-*-*} = $pkgname ]]; then echo $pkgentry return 0 fi @@ -126,12 +126,12 @@ db_write_delta() pkgname="$(get_delta_pkgname $deltafile)" pkgentry=$(find_pkgentry $pkgname) - if [ -z "$pkgentry" ]; then + if [[ -z $pkgentry ]]; then return 1 fi deltas="$pkgentry/deltas" # create deltas file if it does not already exist - if [ ! -f "$deltas" ]; then + if [[ ! -f $deltas ]]; then msg2 "$(gettext "Creating 'deltas' db entry...")" echo -e "%DELTAS%" >>$deltas fi @@ -162,11 +162,11 @@ db_remove_delta() pkgname="$(get_delta_pkgname $deltafile)" pkgentry=$(find_pkgentry $pkgname) - if [ -z "$pkgentry" ]; then + if [[ -z $pkgentry ]]; then return 1 fi deltas="$pkgentry/deltas" - if [ ! -f "$deltas" ]; then + if [[ ! -f $deltas ]]; then return 1 fi if grep -q "$filename" $deltas; then @@ -219,14 +219,14 @@ db_write_entry() csize=$(@SIZECMD@ "$pkgfile") # ensure $pkgname and $pkgver variables were found - if [ -z "$pkgname" -o -z "$pkgver" ]; then + if [[ -z $pkgname || -z $pkgver ]]; then error "$(gettext "Invalid package file '%s'.")" "$pkgfile" return 1 fi cd "$tmpdir" - if [ -d "$pkgname-$pkgver" ]; then + if [[ -d $pkgname-$pkgver ]]; then warning "$(gettext "An entry for '%s' already existed")" "$pkgname-$pkgver" fi @@ -238,30 +238,30 @@ db_write_entry() cd "$pkgname-$pkgver" # restore an eventual deltas file - [ -f "../$pkgname.deltas" ] && mv "../$pkgname.deltas" deltas + [[ -f ../$pkgname.deltas ]] && mv "../$pkgname.deltas" deltas # create desc entry msg2 "$(gettext "Creating 'desc' db entry...")" echo -e "%FILENAME%\n$(basename "$1")\n" >>desc echo -e "%NAME%\n$pkgname\n" >>desc - [ -n "$pkgbase" ] && echo -e "%BASE%\n$pkgbase\n" >>desc + [[ -n $pkgbase ]] && echo -e "%BASE%\n$pkgbase\n" >>desc echo -e "%VERSION%\n$pkgver\n" >>desc - [ -n "$pkgdesc" ] && echo -e "%DESC%\n$pkgdesc\n" >>desc + [[ -n $pkgdesc ]] && echo -e "%DESC%\n$pkgdesc\n" >>desc write_list_entry "GROUPS" "$_groups" "desc" - [ -n "$csize" ] && echo -e "%CSIZE%\n$csize\n" >>desc - [ -n "$size" ] && echo -e "%ISIZE%\n$size\n" >>desc + [[ -n $csize ]] && echo -e "%CSIZE%\n$csize\n" >>desc + [[ -n $size ]] && echo -e "%ISIZE%\n$size\n" >>desc # compute checksums msg2 "$(gettext "Computing md5 checksums...")" echo -e "%MD5SUM%\n$md5sum\n" >>desc - [ -n "$url" ] && echo -e "%URL%\n$url\n" >>desc + [[ -n $url ]] && echo -e "%URL%\n$url\n" >>desc write_list_entry "LICENSE" "$_licenses" "desc" - [ -n "$arch" ] && echo -e "%ARCH%\n$arch\n" >>desc - [ -n "$builddate" ] && echo -e "%BUILDDATE%\n$builddate\n" >>desc - [ -n "$packager" ] && echo -e "%PACKAGER%\n$packager\n" >>desc + [[ -n $arch ]] && echo -e "%ARCH%\n$arch\n" >>desc + [[ -n $builddate ]] && echo -e "%BUILDDATE%\n$builddate\n" >>desc + [[ -n $packager ]] && echo -e "%PACKAGER%\n$packager\n" >>desc write_list_entry "REPLACES" "$_replaces" "desc" - [ -n "$force" ] && echo -e "%FORCE%\n" >>desc + [[ -n $force ]] && echo -e "%FORCE%\n" >>desc # create depends entry msg2 "$(gettext "Creating 'depends' db entry...")" @@ -283,9 +283,9 @@ db_remove_entry() { local pkgname=$1 local notfound=1 local pkgentry=$(find_pkgentry $pkgname) - while [ -n "$pkgentry" ]; do + while [[ -n $pkgentry ]]; do notfound=0 - if [ -f "$pkgentry/deltas" ]; then + if [[ -f $pkgentry/deltas ]]; then mv "$pkgentry/deltas" "$tmpdir/$pkgname.deltas" fi msg2 "$(gettext "Removing existing entry '%s'...")" \ @@ -303,16 +303,16 @@ check_repo_db() CLEAN_LOCK=1 else error "$(gettext "Failed to acquire lockfile: %s.")" "$LOCKFILE" - [ -f "$LOCKFILE" ] && error "$(gettext "Held by process %s")" "$(cat $LOCKFILE)" + [[ -f $LOCKFILE ]] && error "$(gettext "Held by process %s")" "$(cat $LOCKFILE)" exit 1 fi - if [ -f "$REPO_DB_FILE" ]; then + if [[ -f $REPO_DB_FILE ]]; then # there are two situations we can have here- a DB with some entries, # or a DB with no contents at all. if ! bsdtar -tqf "$REPO_DB_FILE" '*/desc' >/dev/null 2>&1; then # check empty case - if [ -n "$(bsdtar -tqf "$REPO_DB_FILE" '*' 2>/dev/null)" ]; then + if [[ -n $(bsdtar -tqf "$REPO_DB_FILE" '*' 2>/dev/null) ]]; then error "$(gettext "Repository file '%s' is not a proper pacman database.")" "$REPO_DB_FILE" exit 1 fi @@ -339,15 +339,15 @@ check_repo_db() add() { - if [ ! -f "$1" ]; then + if [[ ! -f $1 ]]; then error "$(gettext "File '%s' not found.")" "$1" return 1 fi - if [ "${1##*.}" == "delta" ]; then + if [[ ${1##*.} == "delta" ]]; then deltafile=$1 msg "$(gettext "Adding delta '%s'")" "$deltafile" - if [ ! "$(type -p xdelta3)" ]; then + if ! type xdelta3 &>/dev/null; then error "$(gettext "Cannot find the xdelta3 binary! Is xdelta3 installed?")" exit 1 fi @@ -371,7 +371,7 @@ add() remove() { - if [ "${1##*.}" == "delta" ]; then + if [[ ${1##*.} == "delta" ]]; then deltafile=$1 msg "$(gettext "Searching for delta '%s'...")" "$deltafile" if db_remove_delta "$deltafile"; then @@ -405,8 +405,8 @@ clean_up() { local exit_code=$? cd "$startdir" - [ -d "$tmpdir" ] && rm -rf "$tmpdir" - [ $CLEAN_LOCK -eq 1 -a -f "$LOCKFILE" ] && rm -f "$LOCKFILE" + [[ -d $tmpdir ]] && rm -rf "$tmpdir" + (( CLEAN_LOCK )) && [[ -f $LOCKFILE ]] && rm -f "$LOCKFILE" exit $exit_code } @@ -414,7 +414,7 @@ clean_up() { # PROGRAM START # determine whether we have gettext; make it a no-op if we do not -if [ ! $(type -t gettext) ]; then +if ! type gettext &>/dev/null; then gettext() { echo "$@" } @@ -427,7 +427,7 @@ esac # figure out what program we are cmd="$(basename $0)" -if [ "$cmd" != "repo-add" -a "$cmd" != "repo-remove" ]; then +if [[ $cmd != "repo-add" && $cmd != "repo-remove" ]]; then error "$(gettext "Invalid command name '%s' specified.")" "$cmd" exit 1 fi @@ -447,7 +447,7 @@ for arg in "$@"; do case "$arg" in -q|--quiet) QUIET=1;; *) - if [ -z "$REPO_DB_FILE" ]; then + if [[ -z $REPO_DB_FILE ]]; then REPO_DB_FILE="$arg" LOCKFILE="$REPO_DB_FILE.lck" check_repo_db @@ -462,7 +462,7 @@ for arg in "$@"; do done # if at least one operation was a success, re-zip database -if [ $success -eq 1 ]; then +if (( success )); then msg "$(gettext "Creating updated database file '%s'")" "$REPO_DB_FILE" case "$REPO_DB_FILE" in @@ -476,7 +476,7 @@ if [ $success -eq 1 ]; then filename=$(basename "$REPO_DB_FILE") cd "$tmpdir" - if [ -n "$(ls)" ]; then + if [[ -n $(ls) ]]; then bsdtar -c${TAR_OPT}f "$filename" * else # we have no packages remaining? zip up some emptyness @@ -485,8 +485,8 @@ if [ $success -eq 1 ]; then fi cd "$startdir" - [ -f "$REPO_DB_FILE" ] && mv -f "$REPO_DB_FILE" "${REPO_DB_FILE}.old" - [ -f "$tmpdir/$filename" ] && mv "$tmpdir/$filename" "$REPO_DB_FILE" + [[ -f $REPO_DB_FILE ]] && mv -f "$REPO_DB_FILE" "${REPO_DB_FILE}.old" + [[ -f $tmpdir/$filename ]] && mv "$tmpdir/$filename" "$REPO_DB_FILE" else msg "$(gettext "No packages modified, nothing to do.")" exit 1 -- 1.6.5.2
Cedric Staniewski wrote:
FS#16623 suggested this change for makepkg; this patch applies it to the remaining files in the scripts directory.
Signed-off-by: Cedric Staniewski <cedric@gmx.ca> --- scripts/pacman-optimize.sh.in | 22 ++++++------ scripts/pkgdelta.sh.in | 22 ++++++------ scripts/rankmirrors.sh.in | 12 +++--- scripts/repo-add.sh.in | 74 ++++++++++++++++++++-------------------- 4 files changed, 65 insertions(+), 65 deletions(-) <snip>
I went through every single change and they all look correct to me. I would appreciate someone else also doing a review here are there are a lot of changes and I could have overlooked something. The only caution I have is this: - [ $CLEAN_LOCK -eq 1 -a -f "$LOCKFILE" ] && rm -f "$LOCKFILE" + (( CLEAN_LOCK )) && [[ -f $LOCKFILE ]] && rm -f "$LOCKFILE" The (( CLEAN_LOCK )) test is really equivalent of [ $CLEAN_LOCK -ne 0 ] and not [ $CLEAN_LOCK -eq 1 ]. In this case it does not matter as $CLEAN_LOCK can only be assigned 0 or 1 and I can not foresee that changing. But it may pay to be careful. As an aside. Bash tests are confusing... (( 0 )) returns 1 or "false" (( 1 )) returns 0 or "true" Does that not confuse other people too? Allan
On Fri, Nov 06, 2009 at 03:18:23PM +0000, Allan McRae wrote:
Cedric Staniewski wrote:
FS#16623 suggested this change for makepkg; this patch applies it to the remaining files in the scripts directory.
Signed-off-by: Cedric Staniewski <cedric@gmx.ca> --- scripts/pacman-optimize.sh.in | 22 ++++++------ scripts/pkgdelta.sh.in | 22 ++++++------ scripts/rankmirrors.sh.in | 12 +++--- scripts/repo-add.sh.in | 74 ++++++++++++++++++++-------------------- 4 files changed, 65 insertions(+), 65 deletions(-) <snip>
I went through every single change and they all look correct to me. I would appreciate someone else also doing a review here are there are a lot of changes and I could have overlooked something.
The only caution I have is this:
- [ $CLEAN_LOCK -eq 1 -a -f "$LOCKFILE" ] && rm -f "$LOCKFILE" + (( CLEAN_LOCK )) && [[ -f $LOCKFILE ]] && rm -f "$LOCKFILE"
The (( CLEAN_LOCK )) test is really equivalent of [ $CLEAN_LOCK -ne 0 ] and not [ $CLEAN_LOCK -eq 1 ]. In this case it does not matter as $CLEAN_LOCK can only be assigned 0 or 1 and I can not foresee that changing. But it may pay to be careful.
As an aside. Bash tests are confusing... (( 0 )) returns 1 or "false" (( 1 )) returns 0 or "true" Does that not confuse other people too?
Allan
In my original patch for makepkg there were a number of instances where I replaced a test for -eq 1 with a non-zero test. If the variable is assigned always to 0 or 1, this shouldn't be a problem, should it? With the notable exception of SOURCEONLY a lot of variables are boolean. (( 0 )) and (( 1 )) are very much like how C treats integer testing. I just think of (( )) as a C-style test. - Isaac
Isaac Good wrote:
On Fri, Nov 06, 2009 at 03:18:23PM +0000, Allan McRae wrote:
Cedric Staniewski wrote:
FS#16623 suggested this change for makepkg; this patch applies it to the remaining files in the scripts directory.
Signed-off-by: Cedric Staniewski <cedric@gmx.ca> --- scripts/pacman-optimize.sh.in | 22 ++++++------ scripts/pkgdelta.sh.in | 22 ++++++------ scripts/rankmirrors.sh.in | 12 +++--- scripts/repo-add.sh.in | 74 ++++++++++++++++++++-------------------- 4 files changed, 65 insertions(+), 65 deletions(-) <snip>
I went through every single change and they all look correct to me. I would appreciate someone else also doing a review here are there are a lot of changes and I could have overlooked something.
The only caution I have is this:
- [ $CLEAN_LOCK -eq 1 -a -f "$LOCKFILE" ] && rm -f "$LOCKFILE" + (( CLEAN_LOCK )) && [[ -f $LOCKFILE ]] && rm -f "$LOCKFILE"
The (( CLEAN_LOCK )) test is really equivalent of [ $CLEAN_LOCK -ne 0 ] and not [ $CLEAN_LOCK -eq 1 ]. In this case it does not matter as $CLEAN_LOCK can only be assigned 0 or 1 and I can not foresee that changing. But it may pay to be careful.
As an aside. Bash tests are confusing... (( 0 )) returns 1 or "false" (( 1 )) returns 0 or "true" Does that not confuse other people too?
Allan
In my original patch for makepkg there were a number of instances where I replaced a test for -eq 1 with a non-zero test. If the variable is assigned always to 0 or 1, this shouldn't be a problem, should it? With the notable exception of SOURCEONLY a lot of variables are boolean.
Well, SOURCEONLY used to only have values 0 and 1 but then got extended to have 2 as well. This was more flagging that we will need to be careful if we ever expand other variables. Allan
Forgot to point out that the 'type -p anything' tests might not work as intended.
- if [ ! "$(type -p xdelta3)" ]; then + if ! type xdelta3 &>/dev/null; then
$ type asdf bash: type: asdf: not found $ type -p asdf; echo $? 1 $ $ $ touch ~/bin/asdf; chmod +x ~/bin/asdf $ type asdf asdf is /home/makepkg/bin/asdf $ type -p asdf; echo $? /home/makepkg/bin/asdf 0 $ rm ~/bin/asdf $ $ $ asdf() {
echo 1 } $ type asdf asdf is a function asdf () { echo 1 } $ type -p asdf; echo $? 0
That means you cannot rely on the return code when you want to emulate which. You have to test the length of the resulting string instead.
On Fri, Nov 06, 2009 at 08:01:04PM +0000, Cedric Staniewski wrote:
Forgot to point out that the 'type -p anything' tests might not work as intended.
- if [ ! "$(type -p xdelta3)" ]; then + if ! type xdelta3 &>/dev/null; then
$ type asdf bash: type: asdf: not found $ type -p asdf; echo $? 1 $ $ $ touch ~/bin/asdf; chmod +x ~/bin/asdf $ type asdf asdf is /home/makepkg/bin/asdf $ type -p asdf; echo $? /home/makepkg/bin/asdf 0 $ rm ~/bin/asdf $ $ $ asdf() {
echo 1 } $ type asdf asdf is a function asdf () { echo 1 } $ type -p asdf; echo $? 0
That means you cannot rely on the return code when you want to emulate which. You have to test the length of the resulting string instead.
If the command is a bash function, is that not good enough? (ie why the -p) type -pf will suppress functions, so you can do : if type -pf xdelta3 ; then $ type -p q ; echo $? ; q () { echo ; } ; type -pf q ; echo $? ; type -p q ; echo $? 1 1 0 Definitely worth its own patch, though. - Isaac
Isaac Good wrote:
On Fri, Nov 06, 2009 at 08:01:04PM +0000, Cedric Staniewski wrote:
Forgot to point out that the 'type -p anything' tests might not work as intended.
- if [ ! "$(type -p xdelta3)" ]; then + if ! type xdelta3 &>/dev/null; then
$ type asdf bash: type: asdf: not found $ type -p asdf; echo $? 1 $ $ $ touch ~/bin/asdf; chmod +x ~/bin/asdf $ type asdf asdf is /home/makepkg/bin/asdf $ type -p asdf; echo $? /home/makepkg/bin/asdf 0 $ rm ~/bin/asdf $ $ $ asdf() {
echo 1 } $ type asdf asdf is a function asdf () { echo 1 } $ type -p asdf; echo $? 0
That means you cannot rely on the return code when you want to emulate which. You have to test the length of the resulting string instead.
If the command is a bash function, is that not good enough? (ie why the -p) type -pf will suppress functions, so you can do : if type -pf xdelta3 ; then
$ type -p q ; echo $? ; q () { echo ; } ; type -pf q ; echo $? ; type -p q ; echo $? 1 1 0
Definitely worth its own patch, though.
- Isaac
It might be enough, but I wonder, as well, why the -p option is used. I removed it in my patch since it is not needed when testing the return code, but maybe I missed something.
On Tue, Nov 10, 2009 at 2:20 PM, Cedric Staniewski <cedric@gmx.ca> wrote:
Isaac Good wrote:
On Fri, Nov 06, 2009 at 08:01:04PM +0000, Cedric Staniewski wrote:
Forgot to point out that the 'type -p anything' tests might not work as intended.
- if [ ! "$(type -p xdelta3)" ]; then + if ! type xdelta3 &>/dev/null; then
$ type asdf bash: type: asdf: not found $ type -p asdf; echo $? 1 $ $ $ touch ~/bin/asdf; chmod +x ~/bin/asdf $ type asdf asdf is /home/makepkg/bin/asdf $ type -p asdf; echo $? /home/makepkg/bin/asdf 0 $ rm ~/bin/asdf $ $ $ asdf() {
echo 1 } $ type asdf asdf is a function asdf () { echo 1 } $ type -p asdf; echo $? 0
That means you cannot rely on the return code when you want to emulate which. You have to test the length of the resulting string instead.
If the command is a bash function, is that not good enough? (ie why the -p) type -pf will suppress functions, so you can do : if type -pf xdelta3 ; then
$ type -p q ; echo $? ; q () { echo ; } ; type -pf q ; echo $? ; type -p q ; echo $? 1 1 0
Definitely worth its own patch, though.
- Isaac
It might be enough, but I wonder, as well, why the -p option is used. I removed it in my patch since it is not needed when testing the return code, but maybe I missed something.
dmcgee@galway ~/projects/pacman (gpg-more) $ type -p xdelta3; echo $? 1 dmcgee@galway ~/projects/pacman (gpg-more) $ type xdelta3; echo $? -bash: type: xdelta3: not found 1 Note that -p suppresses error messages that you may not want; it's output is only the return code. -Dan
On Tue, Nov 10, 2009 at 02:23:25PM -0600, Dan McGee wrote:
On Tue, Nov 10, 2009 at 2:20 PM, Cedric Staniewski <cedric@gmx.ca> wrote:
It might be enough, but I wonder, as well, why the -p option is used. I removed it in my patch since it is not needed when testing the return code, but maybe I missed something.
dmcgee@galway ~/projects/pacman (gpg-more) $ type -p xdelta3; echo $? 1
dmcgee@galway ~/projects/pacman (gpg-more) $ type xdelta3; echo $? -bash: type: xdelta3: not found 1
Note that -p suppresses error messages that you may not want; it's output is only the return code.
-Dan
The -p only suppresses error messages (STDERR) when it fails. On success it still emits the path to the file on disk (STDOUT). If you want to do the test like this: $ if [ -n "$(type -p xdelta3)" ] ; then then that is the desired behaviour. Output on success, nothing on failure. If you want to just use the return status, you still need to do something with the STDOUT. $ if type -p xdelta3 >/dev/null ; then or $ if type xdelta3 >/dev/null 2>&1 ; then
From f40b051c1c649889a13f35965fa8f8decd505b47 Mon Sep 17 00:00:00 2001 From: Isaac Good <pacman@isaac.otherinbox.com> Date: Wed, 11 Nov 2009 16:47:43 -0500 Subject: [PATCH 1/2] Changing [ to [[ and ((
First half of makepkg This replaces any prior patches of mine Includes stuff like -o to || and -a to && etc if [ $(type .. were preserved due to a bash bug with [[ and set -e and ERR traps Signed-off-by: Isaac Good <pacman@isaac.otherinbox.com> --- scripts/makepkg.sh.in | 202 ++++++++++++++++++++++++------------------------ 1 files changed, 101 insertions(+), 101 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 25fb8d9..8a80d6c 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -112,7 +112,7 @@ error() { # the fakeroot call, the error message will be printed by the main call. ## trap_exit() { - if [ "$INFAKEROOT" -eq 0 ]; then + if (( ! INFAKEROOT )); then echo error "$@" fi @@ -126,21 +126,21 @@ trap_exit() { clean_up() { local EXIT_CODE=$? - if [ "$INFAKEROOT" -eq 1 ]; then + if (( INFAKEROOT )); then # Don't clean up when leaving fakeroot, we're not done yet. return fi - if [ $EXIT_CODE -eq 0 -a "$CLEANUP" -eq 1 ]; then + if (( ! EXIT_CODE && CLEANUP )); then # If it's a clean exit and -c/--clean has been passed... msg "$(gettext "Cleaning up...")" rm -rf "$pkgdir" "$srcdir" - if [ -n "$pkgbase" ]; then + if [[ -n $pkgbase ]]; then # Can't do this unless the BUILDSCRIPT has been sourced. rm -f "${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-build.log"* - if [ "$PKGFUNC" -eq 1 ]; then + if (( PKGFUNC )); then rm -f "${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-package.log"* - elif [ "$SPLITPKG" -eq 1 ]; then + elif (( SPLITPKG )); then for pkg in ${pkgname[@]}; do rm -f "${pkg}-${pkgver}-${pkgrel}-${CARCH}-package.log"* done @@ -190,14 +190,14 @@ get_url() { ## check_option() { local ret=$(in_opt_array "$1" ${options[@]}) - if [ "$ret" != '?' ]; then + if [[ $ret != '?' ]]; then echo $ret return fi # fall back to makepkg.conf options ret=$(in_opt_array "$1" ${OPTIONS[@]}) - if [ "$ret" != '?' ]; then + if [[ $ret != '?' ]]; then echo $ret return fi @@ -231,10 +231,10 @@ in_opt_array() { local opt for opt in "$@"; do opt="${opt,,}" - if [ "$opt" = "$needle" ]; then + if [[ $opt = $needle ]]; then echo 'y' # Enabled return - elif [ "$opt" = "!$needle" ]; then + elif [[ $opt = "!$needle" ]]; then echo 'n' # Disabled return fi @@ -251,10 +251,10 @@ in_opt_array() { ## in_array() { local needle=$1; shift - [ -z "$1" ] && return 1 # Not Found + [[ -z $1 ]] && return 1 # Not Found local item for item in "$@"; do - [ "$item" = "$needle" ] && return 0 # Found + [[ $item = $needle ]] && return 0 # Found done return 1 # Not Found } @@ -268,14 +268,14 @@ get_downloadclient() { local i for i in "${DLAGENTS[@]}"; do local handler="${i%%::*}" - if [ "$proto" = "$handler" ]; then + if [[ $proto = $handler ]]; then agent="${i##*::}" break fi done # if we didn't find an agent, return an error - if [ -z "$agent" ]; then + if [[ -z $agent ]]; then error "$(gettext "There is no agent set up to handle %s URLs. Check %s.")" "$proto" "$MAKEPKG_CONF" plain "$(gettext "Aborting...")" exit 1 # $E_CONFIG_ERROR @@ -283,7 +283,7 @@ get_downloadclient() { # ensure specified program is installed local program="${agent%% *}" - if [ ! -x "$program" ]; then + if [[ ! -x $program ]]; then local baseprog=$(basename $program) error "$(gettext "The download program %s is not installed.")" "$baseprog" plain "$(gettext "Aborting...")" @@ -317,25 +317,25 @@ download_file() { local ret=0 eval "$dlcmd || ret=\$?" - if [ $ret -gt 0 ]; then - [ ! -s "$dlfile" ] && rm -f -- "$dlfile" + if (( ret )); then + [[ ! -s $dlfile ]] && rm -f -- "$dlfile" return $ret fi # rename the temporary download file to the final destination - if [ "$dlfile" != "$file" ]; then + if [[ $dlfile != $file ]]; then mv -f "$SRCDEST/$dlfile" "$SRCDEST/$file" fi } check_deps() { - [ $# -gt 0 ] || return + (( $# )) || return pmout=$(pacman $PACMAN_OPTS -T "$@") ret=$? - if [ $ret -eq 127 ]; then #unresolved deps + if (( ret == 127 )); then #unresolved deps echo "$pmout" - elif [ $ret -ne 0 ]; then + elif (( ret )); then error "$(gettext "Pacman returned a fatal error (%i): %s")" "$ret" "$pmout" exit 1 fi @@ -345,26 +345,26 @@ handle_deps() { local R_DEPS_SATISFIED=0 local R_DEPS_MISSING=1 - [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED local deplist="$*" - if [ "$DEP_BIN" -eq 0 ]; then + if (( ! DEP_BIN )); then return $R_DEPS_MISSING fi - if [ "$DEP_BIN" -eq 1 ]; then + if (( DEP_BIN )); then # install missing deps from binary packages (using pacman -S) msg "$(gettext "Installing missing dependencies...")" local ret=0 - if [ "$ASROOT" -eq 0 ]; then + if (( ! ASROOT )); then sudo pacman $PACMAN_OPTS -S --asdeps $deplist || ret=$? else pacman $PACMAN_OPTS -S --asdeps $deplist || ret=$? fi - if [ $ret -ne 0 ]; then + if (( ret )); then error "$(gettext "Pacman failed to install missing dependencies.")" exit 1 # TODO: error code fi @@ -385,7 +385,7 @@ resolve_deps() { local R_DEPS_MISSING=1 local deplist="$(check_deps $*)" - if [ -z "$deplist" ]; then + if [[ -z $deplist ]]; then return $R_DEPS_SATISFIED fi @@ -393,8 +393,8 @@ resolve_deps() { pkgdeps="$pkgdeps $deplist" # check deps again to make sure they were resolved deplist="$(check_deps $*)" - [ -z "$deplist" ] && return $R_DEPS_SATISFIED - elif [ "$DEP_BIN" -eq 1 ]; then + [[ -z $deplist ]] && return $R_DEPS_SATISFIED + elif (( DEP_BIN )); then error "$(gettext "Failed to install all missing dependencies.")" fi @@ -410,8 +410,8 @@ resolve_deps() { # fix flyspray bug #5923 remove_deps() { # $pkgdeps is a GLOBAL variable, set by resolve_deps() - [ "$RMDEPS" -eq 0 ] && return - [ -z "$pkgdeps" ] && return + (( ! RMDEPS )) && return + [[ -z $pkgdeps ]] && return local dep depstrip deplist deplist="" @@ -422,14 +422,14 @@ remove_deps() { msg "Removing installed dependencies..." local ret=0 - if [ "$ASROOT" -eq 0 ]; then + if (( ! ASROOT )); then sudo pacman $PACMAN_OPTS -Rns $deplist || ret=$? else pacman $PACMAN_OPTS -Rns $deplist || ret=$? fi # Fixes FS#10039 - exit cleanly as package has built successfully - if [ $ret -ne 0 ]; then + if (( ret )); then warning "$(gettext "Failed to remove installed dependencies.")" return 0 fi @@ -438,7 +438,7 @@ remove_deps() { download_sources() { msg "$(gettext "Retrieving Sources...")" - if [ ! -w "$SRCDEST" ] ; then + if [[ ! -w $SRCDEST ]] ; then error "$(gettext "You do not have write permission to store downloads in %s.")" "$SRCDEST" plain "$(gettext "Aborting...")" exit 1 @@ -450,12 +450,12 @@ download_sources() { for netfile in "${source[@]}"; do local file=$(get_filename "$netfile") local url=$(get_url "$netfile") - if [ -f "$startdir/$file" ]; then + if [[ -f "$startdir/$file" ]]; then msg2 "$(gettext "Found %s in build dir")" "$file" rm -f "$srcdir/$file" ln -s "$startdir/$file" "$srcdir/" continue - elif [ -f "$SRCDEST/$file" ]; then + elif [[ -f "$SRCDEST/$file" ]]; then msg2 "$(gettext "Using cached copy of %s")" "$file" rm -f "$srcdir/$file" ln -s "$SRCDEST/$file" "$srcdir/" @@ -463,7 +463,7 @@ download_sources() { fi # if we get here, check to make sure it was a URL, else fail - if [ "$file" = "$url" ]; then + if [[ $file = $url ]]; then error "$(gettext "%s was not found in the build directory and is not a URL.")" "$file" exit 1 # $E_MISSING_FILE fi @@ -475,7 +475,7 @@ download_sources() { # fix flyspray bug #3289 local ret=0 download_file "$dlclient" "$url" "$file" || ret=$? - if [ $ret -gt 0 ]; then + if (( ret )); then error "$(gettext "Failure while downloading %s")" "$file" plain "$(gettext "Aborting...")" exit 1 @@ -512,7 +512,7 @@ generate_checksums() { local i=0; local indent='' - while [ $i -lt $((${#integ}+6)) ]; do + while [[ $i -lt $((${#integ}+6)) ]]; do indent="$indent " i=$(($i+1)) done @@ -521,8 +521,8 @@ generate_checksums() { for netfile in "${source[@]}"; do local file="$(get_filename "$netfile")" - if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then error "$(gettext "Unable to find source file %s to generate checksum.")" "$file" plain "$(gettext "Aborting...")" exit 1 @@ -533,10 +533,10 @@ generate_checksums() { local sum="$(openssl dgst -${integ} "$file")" sum=${sum##* } - [ $ct -gt 0 ] && echo -n "$indent" + (( ct )) && echo -n $indent echo -n "'$sum'" ct=$(($ct+1)) - [ $ct -lt $numsrc ] && echo + (( $ct < $numsrc )) && echo done echo ")" @@ -544,7 +544,7 @@ generate_checksums() { } check_checksums() { - [ ${#source[@]} -eq 0 ] && return 0 + (( ! ${#source[@]} )) && return 0 if [ ! $(type -p openssl) ]; then error "$(gettext "Cannot find openssl.")" @@ -555,7 +555,7 @@ check_checksums() { local integ required for integ in md5 sha1 sha256 sha384 sha512; do local integrity_sums=($(eval echo "\${${integ}sums[@]}")) - if [ ${#integrity_sums[@]} -eq ${#source[@]} ]; then + if (( ${#integrity_sums[@]} == ${#source[@]} )); then msg "$(gettext "Validating source files with %s...")" "${integ}sums" correlation=1 local errors=0 @@ -566,8 +566,8 @@ check_checksums() { file="$(get_filename "$file")" echo -n " $file ... " >&2 - if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then echo "$(gettext "NOT FOUND")" >&2 errors=1 found=0 @@ -576,11 +576,11 @@ check_checksums() { fi fi - if [ $found -gt 0 ] ; then + if (( $found )) ; then local expectedsum="${integrity_sums[$idx],,}" local realsum="$(openssl dgst -${integ} "$file")" realsum="${realsum##* }" - if [ "$expectedsum" = "$realsum" ]; then + if [[ $expectedsum = $realsum ]]; then echo "$(gettext "Passed")" >&2 else echo "$(gettext "FAILED")" >&2 @@ -591,18 +591,18 @@ check_checksums() { idx=$((idx + 1)) done - if [ $errors -gt 0 ]; then + if (( errors )); then error "$(gettext "One or more files did not pass the validity check!")" exit 1 # TODO: error code fi - elif [ ${#integrity_sums[@]} -gt 0 ]; then + elif (( ${#integrity_sums[@]} )); then error "$(gettext "Integrity checks (%s) differ in size from the source array.")" "$integ" exit 1 # TODO: error code fi done - if [ $correlation -eq 0 ]; then - if [ $SKIPINTEG -eq 1 ]; then + if (( ! correlation )); then + if (( SKIPINTEG )); then warning "$(gettext "Integrity checks are missing.")" else error "$(gettext "Integrity checks are missing.")" @@ -622,8 +622,8 @@ extract_sources() { continue fi - if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then error "$(gettext "Unable to find source file %s for extraction.")" "$file" plain "$(gettext "Aborting...")" exit 1 @@ -662,31 +662,31 @@ extract_sources() { local ret=0 msg2 "$(gettext "Extracting %s with %s")" "$file" "$cmd" - if [ "$cmd" = "bsdtar" ]; then + if [[ $cmd = bsdtar ]]; then $cmd -xf "$file" || ret=? else rm -f "${file%.*}" $cmd -dcf "$file" > "${file%.*}" || ret=? fi - if [ $ret -ne 0 ]; then + if (( ret )); then error "$(gettext "Failed to extract %s")" "$file" plain "$(gettext "Aborting...")" exit 1 fi done - if [ $EUID -eq 0 ]; then + if (( EUID == 0 )); then # change perms of all source files to root user & root group chown -R 0:0 "$srcdir" fi } error_function() { - if [ -p "$logpipe" ]; then + if [[ -p $logpipe ]]; then rm "$logpipe" fi # first exit all subshells, then print the error - if [ $BASH_SUBSHELL -eq 0 ]; then + if (( ! BASH_SUBSHELL )); then plain "$(gettext "Aborting...")" remove_deps fi @@ -694,13 +694,13 @@ error_function() { } run_function() { - if [ -z "$1" ]; then + if [[ -z $1 ]]; then return 1 fi pkgfunc="$1" # clear user-specified makeflags if requested - if [ "$(check_option makeflags)" = "n" ]; then + if [[ "$(check_option makeflags)" = "n" ]]; then MAKEFLAGS="" fi @@ -713,12 +713,12 @@ run_function() { local shellopts=$(shopt -p) local ret=0 - if [ "$LOGGING" -eq 1 ]; then + if (( LOGGING )); then BUILDLOG="${startdir}/${pkgname}-${pkgver}-${pkgrel}-${CARCH}-$pkgfunc.log" - if [ -f "$BUILDLOG" ]; then + if [[ -f $BUILDLOG ]]; then local i=1 while true; do - if [ -f "$BUILDLOG.$i" ]; then + if [[ -f "$BUILDLOG.$i" ]]; then i=$(($i +1)) else break @@ -752,24 +752,24 @@ run_function() { run_build() { # use distcc if it is requested (check buildenv and PKGBUILD opts) - if [ "$(check_buildenv distcc)" = "y" -a "$(check_option distcc)" != "n" ]; then - [ -d /usr/lib/distcc/bin ] && export PATH="/usr/lib/distcc/bin:$PATH" + if [[ "$(check_buildenv distcc)" = "y" && "$(check_option distcc)" != "n" ]]; then + [[ -d /usr/lib/distcc/bin ]] && export PATH="/usr/lib/distcc/bin:$PATH" export DISTCC_HOSTS - elif [ "$(check_option distcc)" = "n" ]; then + elif [[ "$(check_option distcc)" = "n" ]]; then # if it is not wanted, clear the makeflags too MAKEFLAGS="" fi # use ccache if it is requested (check buildenv and PKGBUILD opts) - if [ "$(check_buildenv ccache)" = "y" -a "$(check_option ccache)" != "n" ]; then - [ -d /usr/lib/ccache/bin ] && export PATH="/usr/lib/ccache/bin:$PATH" + if [[ "$(check_buildenv ccache)" = "y" && "$(check_option ccache)" != "n" ]]; then + [[ -d /usr/lib/ccache/bin ]] && export PATH="/usr/lib/ccache/bin:$PATH" fi run_function "build" } run_package() { - if [ -z "$1" ]; then + if [[ -z $1 ]]; then pkgfunc="package" else pkgfunc="package_$1" @@ -782,16 +782,16 @@ tidy_install() { cd "$pkgdir" msg "$(gettext "Tidying install...")" - if [ "$(check_option docs)" = "n" -a -n "${DOC_DIRS[*]}" ]; then + if [[ "$(check_option docs)" = "n" && -n "${DOC_DIRS[*]}" ]]; then msg2 "$(gettext "Removing doc files...")" rm -rf ${DOC_DIRS[@]} fi - if [ "$(check_option purge)" = "y" -a -n "${PURGE_TARGETS[*]}" ]; then + if [[ "$(check_option purge)" = "y" && -n "${PURGE_TARGETS[*]}" ]]; then msg2 "$(gettext "Purging other files...")" local pt for pt in "${PURGE_TARGETS[@]}"; do - if [ "${pt}" = "${pt//\/}" ]; then + if [[ "${pt}" = "${pt//\/}" ]]; then find . -type f -name "${pt}" -exec rm -f -- '{}' \; else rm -f ${pt} @@ -799,16 +799,16 @@ tidy_install() { done fi - if [ "$(check_option zipman)" = "y" -a -n "${MAN_DIRS[*]}" ]; then + if [[ "$(check_option zipman)" = "y" && -n "${MAN_DIRS[*]}" ]]; then msg2 "$(gettext "Compressing man and info pages...")" local manpage ext file link hardlinks hl find ${MAN_DIRS[@]} -type f 2>/dev/null | while read manpage ; do # check file still exists (potentially compressed with hard link) - if [ -f ${manpage} ]; then + if [[ -f ${manpage} ]]; then ext="${manpage##*.}" file="${manpage##*/}" - if [ "$ext" != "gz" -a "$ext" != "bz2" ]; then + if [[ $ext != gz && $ext != bz2 ]]; then # update symlinks to this manpage find ${MAN_DIRS[@]} -lname "$file" 2>/dev/null | while read link ; do @@ -834,7 +834,7 @@ tidy_install() { done fi - if [ "$(check_option strip)" = "y" -a -n "${STRIP_DIRS[*]}" ]; then + if [[ $(check_option strip) = y && -n ${STRIP_DIRS[*]} ]]; then msg2 "$(gettext "Stripping debugging symbols from binaries and libraries...")" local binary find ${STRIP_DIRS[@]} -type f 2>/dev/null | while read binary ; do @@ -851,12 +851,12 @@ tidy_install() { done fi - if [ "$(check_option libtool)" = "n" ]; then + if [[ "$(check_option libtool)" = "n" ]]; then msg2 "$(gettext "Removing libtool .la files...")" find . ! -type d -name "*.la" -exec rm -f -- '{}' \; fi - if [ "$(check_option emptydirs)" = "n" ]; then + if [[ "$(check_option emptydirs)" = "n" ]]; then msg2 "$(gettext "Removing empty directories...")" find . -depth -type d -empty -delete fi @@ -864,7 +864,7 @@ tidy_install() { write_pkginfo() { local builddate=$(date -u "+%s") - if [ -n "$PACKAGER" ]; then + if [[ -n $PACKAGER ]]; then local packager="$PACKAGER" else local packager="Unknown Packager" @@ -874,12 +874,12 @@ write_pkginfo() { msg2 "$(gettext "Generating .PKGINFO file...")" echo "# Generated by makepkg $myver" >.PKGINFO - if [ "$INFAKEROOT" -eq 1 ]; then + if (( INFAKEROOT )); then echo "# using $(fakeroot -v)" >>.PKGINFO fi echo "# $(LC_ALL=C date -u)" >>.PKGINFO echo "pkgname = $1" >>.PKGINFO - [ "$SPLITPKG" -eq 1 ] && echo "pkgbase = $pkgbase" >>.PKGINFO + (( SPLITPKG )) && echo pkgbase = $pkgbase >>.PKGINFO echo "pkgver = $pkgver-$pkgrel" >>.PKGINFO echo "pkgdesc = $pkgdesc" >>.PKGINFO echo "url = $url" >>.PKGINFO @@ -887,7 +887,7 @@ write_pkginfo() { echo "packager = $packager" >>.PKGINFO echo "size = $size" >>.PKGINFO echo "arch = $PKGARCH" >>.PKGINFO - if [ "$(check_option force)" = "y" ]; then + if [[ "$(check_option force)" = "y" ]]; then echo "force = true" >> .PKGINFO fi @@ -918,8 +918,8 @@ write_pkginfo() { done for it in "${packaging_options[@]}"; do local ret="$(check_option $it)" - if [ "$ret" != "?" ]; then - if [ "$ret" = "y" ]; then + if [[ $ret != "?" ]]; then + if [[ $ret = y ]]; then echo "makepkgopt = $it" >>.PKGINFO else echo "makepkgopt = !$it" >>.PKGINFO @@ -929,7 +929,7 @@ write_pkginfo() { # TODO maybe remove this at some point # warn if license array is not present or empty - if [ -z "$license" ]; then + if [[ -z $license ]]; then warning "$(gettext "Please add a license line to your %s!")" "$BUILDSCRIPT" plain "$(gettext "Example for GPL\'ed software: license=('GPL').")" fi @@ -941,14 +941,14 @@ check_package() { # check existence of backup files local file for file in "${backup[@]}"; do - if [ ! -f "$file" ]; then + if [[ ! -f $file ]]; then warning "$(gettext "Invalid backup entry : %s")" "$file" fi done } create_package() { - if [ ! -d "$pkgdir" ]; then + if [[ ! -d $pkgdir ]]; then error "$(gettext "Missing pkg/ directory.")" plain "$(gettext "Aborting...")" exit 1 # $E_MISSING_PKGDIR @@ -959,13 +959,13 @@ create_package() { cd "$pkgdir" msg "$(gettext "Creating package...")" - if [ -z "$1" ]; then + if [[ -z $1 ]]; then nameofpkg="$pkgname" else nameofpkg="$1" fi - if [ "$arch" = "any" ]; then + if [[ $arch = "any" ]]; then PKGARCH="any" else PKGARCH=$CARCH @@ -976,14 +976,14 @@ create_package() { local comp_files=".PKGINFO" # check for an install script - if [ -n "$install" ]; then + if [[ -n $install ]]; then msg2 "$(gettext "Adding install script...")" cp "$startdir/$install" .INSTALL comp_files="$comp_files .INSTALL" fi # do we have a changelog? - if [ -n "$changelog" ]; then + if [[ -n $changelog ]]; then msg2 "$(gettext "Adding package changelog...")" cp "$startdir/$changelog" .CHANGELOG comp_files="$comp_files .CHANGELOG" @@ -1009,7 +1009,7 @@ create_package() { bsdtar -cf - $comp_files * > "$pkg_file" || ret=$? shopt -u nullglob - if [ $ret -eq 0 ]; then + if (( ! ret )); then case "$PKGEXT" in *tar.gz) gzip -f -n "$pkg_file" ;; *tar.bz2) bzip2 -f "$pkg_file" ;; @@ -1018,7 +1018,7 @@ create_package() { ret=$? fi - if [ $ret -ne 0 ]; then + if (( ret )); then error "$(gettext "Failed to create package file.")" exit 1 # TODO: error code fi @@ -1042,8 +1042,8 @@ create_srcpackage() { msg2 "$(gettext "Adding %s...")" "$BUILDSCRIPT" ln -s "${BUILDFILE}" "${srclinks}/${pkgbase}/${BUILDSCRIPT}" - if [ -n "$install" ]; then - if [ -f $install ]; then + if [[ -n $install ]]; then + if [[ -f $install ]]; then msg2 "$(gettext "Adding install script...")" ln -s "${startdir}/$install" "${srclinks}/${pkgbase}/" else @@ -1051,8 +1051,8 @@ create_srcpackage() { fi fi - if [ -n "$changelog" ]; then - if [ -f "$changelog" ]; then + if [[ -n $changelog ]]; then + if [[ -f $changelog ]]; then msg2 "$(gettext "Adding package changelog...")" ln -s "${startdir}/$changelog" "${srclinks}/${pkgbase}/" else @@ -1063,10 +1063,10 @@ create_srcpackage() { local netfile for netfile in "${source[@]}"; do local file=$(get_filename "$netfile") - if [ -f "$netfile" ]; then + if [[ -f $netfile ]]; then msg2 "$(gettext "Adding %s...")" "$netfile" ln -s "${startdir}/$netfile" "${srclinks}/${pkgbase}" - elif [ "$SOURCEONLY" -eq 2 -a -f "$SRCDEST/$file" ]; then + elif (( SOURCEONLY == 2 )) && [[ -f "$SRCDEST/$file" ]]; then msg2 "$(gettext "Adding %s...")" "$file" ln -s "$SRCDEST/$file" "${srclinks}/${pkgbase}/" fi -- 1.6.5.2
Isaac Good wrote:
From f40b051c1c649889a13f35965fa8f8decd505b47 Mon Sep 17 00:00:00 2001 From: Isaac Good <pacman@isaac.otherinbox.com> Date: Wed, 11 Nov 2009 16:47:43 -0500 Subject: [PATCH 1/2] Changing [ to [[ and ((
First half of makepkg This replaces any prior patches of mine Includes stuff like -o to || and -a to && etc if [ $(type .. were preserved due to a bash bug with [[ and set -e and ERR traps
Signed-off-by: Isaac Good <pacman@isaac.otherinbox.com> ---
I hope your patch do not get lost in this thread.
@@ -345,26 +345,26 @@ handle_deps() { local R_DEPS_SATISFIED=0 local R_DEPS_MISSING=1
- [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED
Is there a reason why you do not use (( ! $# )) here?
@@ -450,12 +450,12 @@ download_sources() { for netfile in "${source[@]}"; do local file=$(get_filename "$netfile") local url=$(get_url "$netfile") - if [ -f "$startdir/$file" ]; then + if [[ -f "$startdir/$file" ]]; then msg2 "$(gettext "Found %s in build dir")" "$file" rm -f "$srcdir/$file" ln -s "$startdir/$file" "$srcdir/" continue - elif [ -f "$SRCDEST/$file" ]; then + elif [[ -f "$SRCDEST/$file" ]]; then msg2 "$(gettext "Using cached copy of %s")" "$file" rm -f "$srcdir/$file" ln -s "$SRCDEST/$file" "$srcdir/"
We could remove the quotes here, too.
@@ -512,7 +512,7 @@ generate_checksums() {
local i=0; local indent='' - while [ $i -lt $((${#integ}+6)) ]; do + while [[ $i -lt $((${#integ}+6)) ]]; do indent="$indent " i=$(($i+1)) done
What about "while (( $i < $((${#integ}+6)) )); do"?
@@ -521,8 +521,8 @@ generate_checksums() { for netfile in "${source[@]}"; do local file="$(get_filename "$netfile")"
- if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then error "$(gettext "Unable to find source file %s to generate checksum.")" "$file" plain "$(gettext "Aborting...")" exit 1
Same as above. Quotes are not necessary.
@@ -533,10 +533,10 @@ generate_checksums() {
local sum="$(openssl dgst -${integ} "$file")" sum=${sum##* } - [ $ct -gt 0 ] && echo -n "$indent" + (( ct )) && echo -n $indent echo -n "'$sum'"
I do not think we want to remove the quotes here. $ indent=" " $ echo -n $indent; echo asdf asdf $ echo -n "$indent"; echo asdf asdf $
@@ -566,8 +566,8 @@ check_checksums() { file="$(get_filename "$file")" echo -n " $file ... " >&2
- if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then echo "$(gettext "NOT FOUND")" >&2 errors=1 found=0 @@ -622,8 +622,8 @@ extract_sources() { continue fi
- if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then error "$(gettext "Unable to find source file %s for extraction.")" "$file" plain "$(gettext "Aborting...")" exit 1
Quotes are not necessary.
@@ -662,31 +662,31 @@ extract_sources() { ... fi done
- if [ $EUID -eq 0 ]; then + if (( EUID == 0 )); then # change perms of all source files to root user & root group chown -R 0:0 "$srcdir" fi }
...
(( ! EUID )) ?
@@ -713,12 +713,12 @@ run_function() { ... local i=1 while true; do - if [ -f "$BUILDLOG.$i" ]; then + if [[ -f "$BUILDLOG.$i" ]]; then i=$(($i +1)) else break
Quotes again? There are several other places where quotes or the dollar signs (in e.g. (( $found )) ) are not needed. Maybe you want to remove them, maybe not. Cedric
On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:
I hope your patch do not get lost in this thread.
Erm. Yeah, I hope so too. How does it get threaded?
@@ -345,26 +345,26 @@ handle_deps() { local R_DEPS_SATISFIED=0 local R_DEPS_MISSING=1
- [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED
Is there a reason why you do not use (( ! $# )) here?
Some variables are used as a boolean flag in which case testing if ((VAR)) and if (( ! VAR )) reads well. The $# (and EUID) are used as integer values so it struck me as more understandable or readable in this form.
@@ -450,12 +450,12 @@ download_sources() { for netfile in "${source[@]}"; do local file=$(get_filename "$netfile") local url=$(get_url "$netfile") - if [ -f "$startdir/$file" ]; then + if [[ -f "$startdir/$file" ]]; then msg2 "$(gettext "Found %s in build dir")" "$file" rm -f "$srcdir/$file" ln -s "$startdir/$file" "$srcdir/" continue - elif [ -f "$SRCDEST/$file" ]; then + elif [[ -f "$SRCDEST/$file" ]]; then msg2 "$(gettext "Using cached copy of %s")" "$file" rm -f "$srcdir/$file" ln -s "$SRCDEST/$file" "$srcdir/"
We could remove the quotes here, too.
I was lax with some of the quotes when doing concatenation and parameter expansion...
@@ -512,7 +512,7 @@ generate_checksums() {
local i=0; local indent='' - while [ $i -lt $((${#integ}+6)) ]; do + while [[ $i -lt $((${#integ}+6)) ]]; do indent="$indent " i=$(($i+1)) done
What about "while (( $i < $((${#integ}+6)) )); do"?
I was planning a separate patch switching to a for loop. for (( i = 0; i < ${#integ} + 6; i++ ))
@@ -521,8 +521,8 @@ generate_checksums() { for netfile in "${source[@]}"; do local file="$(get_filename "$netfile")"
- if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then error "$(gettext "Unable to find source file %s to generate checksum.")" "$file" plain "$(gettext "Aborting...")" exit 1
Same as above. Quotes are not necessary.
@@ -533,10 +533,10 @@ generate_checksums() {
local sum="$(openssl dgst -${integ} "$file")" sum=${sum##* } - [ $ct -gt 0 ] && echo -n "$indent" + (( ct )) && echo -n $indent echo -n "'$sum'"
I do not think we want to remove the quotes here.
$ indent=" " $ echo -n $indent; echo asdf asdf $ echo -n "$indent"; echo asdf asdf $
That'd be a careless use of s/"// in a macro on my behalf. Thanks for catching.
@@ -566,8 +566,8 @@ check_checksums() { file="$(get_filename "$file")" echo -n " $file ... " >&2
- if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then echo "$(gettext "NOT FOUND")" >&2 errors=1 found=0 @@ -622,8 +622,8 @@ extract_sources() { continue fi
- if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then error "$(gettext "Unable to find source file %s for extraction.")" "$file" plain "$(gettext "Aborting...")" exit 1
Quotes are not necessary.
@@ -662,31 +662,31 @@ extract_sources() { ... fi done
- if [ $EUID -eq 0 ]; then + if (( EUID == 0 )); then # change perms of all source files to root user & root group chown -R 0:0 "$srcdir" fi }
...
(( ! EUID )) ?
As above
@@ -713,12 +713,12 @@ run_function() { ... local i=1 while true; do - if [ -f "$BUILDLOG.$i" ]; then + if [[ -f "$BUILDLOG.$i" ]]; then i=$(($i +1)) else break
Quotes again? There are several other places where quotes or the dollar signs (in e.g. (( $found )) ) are not needed. Maybe you want to remove them, maybe not.
Cedric
Should I implement these changes and resend this patch? /me goes looking for git help Thanks for the feedback! - Isaac
Isaac Good wrote:
On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:
I hope your patch do not get lost in this thread.
Erm. Yeah, I hope so too. How does it get threaded?
I guess you hit the reply button instead of writing a new mail to pacman-dev. At least, there is the In-Reply-To header field in your mail header.
@@ -345,26 +345,26 @@ handle_deps() { local R_DEPS_SATISFIED=0 local R_DEPS_MISSING=1
- [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED
Is there a reason why you do not use (( ! $# )) here?
Some variables are used as a boolean flag in which case testing if ((VAR)) and if (( ! VAR )) reads well. The $# (and EUID) are used as integer values so it struck me as more understandable or readable in this form.
I am fine with that, but would it not makes sense to use (( $# > 0 )) instead of just (( $# )) then?
@@ -512,7 +512,7 @@ generate_checksums() {
local i=0; local indent='' - while [ $i -lt $((${#integ}+6)) ]; do + while [[ $i -lt $((${#integ}+6)) ]]; do indent="$indent " i=$(($i+1)) done What about "while (( $i < $((${#integ}+6)) )); do"?
I was planning a separate patch switching to a for loop. for (( i = 0; i < ${#integ} + 6; i++ ))
In this case, I would either get rid of -lt by converting to a for loop directly or not touch that line at all in this patch.
Should I implement these changes and resend this patch? /me goes looking for git help
If you like, but you could also wait for at least Allan's comment. You can use "git commit --amend" to alter your existing commit in git by the way.
On Thu, Nov 12, 2009 at 10:37:31PM +0000, Cedric Staniewski wrote:
Isaac Good wrote:
On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:
I hope your patch do not get lost in this thread.
Erm. Yeah, I hope so too. How does it get threaded?
I guess you hit the reply button instead of writing a new mail to pacman-dev. At least, there is the In-Reply-To header field in your mail header.
@@ -345,26 +345,26 @@ handle_deps() { local R_DEPS_SATISFIED=0 local R_DEPS_MISSING=1
- [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED
Is there a reason why you do not use (( ! $# )) here?
Some variables are used as a boolean flag in which case testing if ((VAR)) and if (( ! VAR )) reads well. The $# (and EUID) are used as integer values so it struck me as more understandable or readable in this form.
I am fine with that, but would it not makes sense to use (( $# > 0 )) instead of just (( $# )) then?
I am not sure I understand what you are saying. In general there are no negative values. With boolean flags either it is set or not set. So you get : if (( FLAG )) and if (( ! FLAG )) With $# (and EUID) the code is set up to test for zero, not non-zero. (( $# > 0 )) and (( $# )) are the reverse of what the code checks for. All of these do the same thing: $ (( $# == 0 )) && return $R_DEPS_SATISFIED $ (( $# > 0 )) || return $R_DEPS_SATISFIED $ (( ! $# )) && return $R_DEPS_SATISFIED $ (( $# )) || return $R_DEPS_SATISFIED I left the test as && because there was no compelling reason to change it. That aside, I am in favor of either of the first two for $# as opposed to the latter two for boolean flags.
@@ -512,7 +512,7 @@ generate_checksums() {
local i=0; local indent='' - while [ $i -lt $((${#integ}+6)) ]; do + while [[ $i -lt $((${#integ}+6)) ]]; do indent="$indent " i=$(($i+1)) done What about "while (( $i < $((${#integ}+6)) )); do"?
I was planning a separate patch switching to a for loop. for (( i = 0; i < ${#integ} + 6; i++ ))
In this case, I would either get rid of -lt by converting to a for loop directly or not touch that line at all in this patch.
amended/reversed
Should I implement these changes and resend this patch? /me goes looking for git help
If you like, but you could also wait for at least Allan's comment. You can use "git commit --amend" to alter your existing commit in git by the way.
Thanks. I'm learning a lot of git today ;) - Isaac
Isaac Good wrote:
On Thu, Nov 12, 2009 at 10:37:31PM +0000, Cedric Staniewski wrote:
Isaac Good wrote:
On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:
I hope your patch do not get lost in this thread. Erm. Yeah, I hope so too. How does it get threaded?
I guess you hit the reply button instead of writing a new mail to pacman-dev. At least, there is the In-Reply-To header field in your mail header.
@@ -345,26 +345,26 @@ handle_deps() { local R_DEPS_SATISFIED=0 local R_DEPS_MISSING=1
- [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED
Is there a reason why you do not use (( ! $# )) here? Some variables are used as a boolean flag in which case testing if ((VAR)) and if (( ! VAR )) reads well. The $# (and EUID) are used as integer values so it struck me as more understandable or readable in this form.
I am fine with that, but would it not makes sense to use (( $# > 0 )) instead of just (( $# )) then?
I am not sure I understand what you are saying. In general there are no negative values. With boolean flags either it is set or not set. So you get : if (( FLAG )) and if (( ! FLAG )) With $# (and EUID) the code is set up to test for zero, not non-zero. (( $# > 0 )) and (( $# )) are the reverse of what the code checks for. All of these do the same thing: $ (( $# == 0 )) && return $R_DEPS_SATISFIED $ (( $# > 0 )) || return $R_DEPS_SATISFIED $ (( ! $# )) && return $R_DEPS_SATISFIED $ (( $# )) || return $R_DEPS_SATISFIED
I left the test as && because there was no compelling reason to change it. That aside, I am in favor of either of the first two for $# as opposed to the latter two for boolean flags.
Sorry, I meant another line from your patch where you use (( $# )).
}
check_deps() { - [ $# -gt 0 ] || return + (( $# )) || return
pmout=$(pacman $PACMAN_OPTS -T "$@") ret=$? - if [ $ret -eq 127 ]; then #unresolved deps + if (( ret == 127 )); then #unresolved deps echo "$pmout" - elif [ $ret -ne 0 ]; then + elif (( ret )); then error "$(gettext "Pacman returned a fatal error (%i): %s")" "$ret" "$pmout" exit 1 fi
ret is another non-boolean variable by the way. I think some kind of code style guide is a good idea. :)
On Thu, Nov 12, 2009 at 06:29:11PM -0500, Isaac Good wrote:
I left the test as && because there was no compelling reason to change it.
I've been reading the various [ ( vs [[ (( threads and still cannot find a compelling reason for any of it. It makes the code much less readable to the people who have been writing portable or semi-portable scripts for years. Makepkg doesn't require sh portability, but seeing all the back and forth questions, I'd say it doesn't require the extra confusion and opacity. It seems clear that even the majority of respondents have different ideas of what the proper syntax should be whereas with the old way, people read it and just know what it is doing. If I am missing something, could someone please enlighten me? Thanks! -- Jeff My other computer is an abacus.
On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:
@@ -450,12 +450,12 @@ download_sources() { for netfile in "${source[@]}"; do local file=$(get_filename "$netfile") local url=$(get_url "$netfile") - if [ -f "$startdir/$file" ]; then + if [[ -f "$startdir/$file" ]]; then msg2 "$(gettext "Found %s in build dir")" "$file" rm -f "$srcdir/$file" ln -s "$startdir/$file" "$srcdir/" continue - elif [ -f "$SRCDEST/$file" ]; then + elif [[ -f "$SRCDEST/$file" ]]; then msg2 "$(gettext "Using cached copy of %s")" "$file" rm -f "$srcdir/$file" ln -s "$SRCDEST/$file" "$srcdir/"
What about something like this?
if [[ "$(check_option makeflags)" = "n" ]]; then
None of the quotes are needed. I'm inclined to replace it with:
if [[ $(check_option makeflags) = "n" ]]; then
While the quotes around n are not needed, they highlight that n is a string which appeals to me since it looks like a string as found in other languages. Or would you say to drop all the quotes. Maybe some style guidelines are needed for the bash language... - Isaac
Isaac Good wrote:
On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:
@@ -450,12 +450,12 @@ download_sources() { for netfile in "${source[@]}"; do local file=$(get_filename "$netfile") local url=$(get_url "$netfile") - if [ -f "$startdir/$file" ]; then + if [[ -f "$startdir/$file" ]]; then msg2 "$(gettext "Found %s in build dir")" "$file" rm -f "$srcdir/$file" ln -s "$startdir/$file" "$srcdir/" continue - elif [ -f "$SRCDEST/$file" ]; then + elif [[ -f "$SRCDEST/$file" ]]; then msg2 "$(gettext "Using cached copy of %s")" "$file" rm -f "$srcdir/$file" ln -s "$SRCDEST/$file" "$srcdir/"
What about something like this?
if [[ "$(check_option makeflags)" = "n" ]]; then
None of the quotes are needed. I'm inclined to replace it with:
if [[ $(check_option makeflags) = "n" ]]; then
While the quotes around n are not needed, they highlight that n is a string which appeals to me since it looks like a string as found in other languages. Or would you say to drop all the quotes.
Aren't the quotes around the string on the right required if it does contain spaces? Anyway, I would prefer all such strings to be quoted so I am happy with your suggestion. Allan
Cedric Staniewski wrote:
Isaac Good wrote:
From f40b051c1c649889a13f35965fa8f8decd505b47 Mon Sep 17 00:00:00 2001 From: Isaac Good <pacman@isaac.otherinbox.com> Date: Wed, 11 Nov 2009 16:47:43 -0500 Subject: [PATCH 1/2] Changing [ to [[ and ((
First half of makepkg This replaces any prior patches of mine Includes stuff like -o to || and -a to && etc if [ $(type .. were preserved due to a bash bug with [[ and set -e and ERR traps
Signed-off-by: Isaac Good <pacman@isaac.otherinbox.com> ---
I hope your patch do not get lost in this thread.
I have the patches marked so that they will not be lost.
@@ -450,12 +450,12 @@ download_sources() { for netfile in "${source[@]}"; do local file=$(get_filename "$netfile") local url=$(get_url "$netfile") - if [ -f "$startdir/$file" ]; then + if [[ -f "$startdir/$file" ]]; then msg2 "$(gettext "Found %s in build dir")" "$file" rm -f "$srcdir/$file" ln -s "$startdir/$file" "$srcdir/" continue - elif [ -f "$SRCDEST/$file" ]; then + elif [[ -f "$SRCDEST/$file" ]]; then msg2 "$(gettext "Using cached copy of %s")" "$file" rm -f "$srcdir/$file" ln -s "$SRCDEST/$file" "$srcdir/"
We could remove the quotes here, too.
Hmmm... can we. I know we can when there are spaces in a single variable, but for some reason I thought quotes were needed when joining multiple variables like that. I could be wrong... Allan
Allan McRae wrote:
Cedric Staniewski wrote:
We could remove the quotes here, too.
Hmmm... can we. I know we can when there are spaces in a single variable, but for some reason I thought quotes were needed when joining multiple variables like that. I could be wrong...
[[ behaves quite different compared to [ in this aspect, but as far as I know, you do not need quotes to join variables as long as there are no spaces between the variables ;). There are several exceptions though, like cd for example, which require quotes even for single variables when they contain spaces. I think this has something to do with the type of the command. Builtins/external commands (usually) need quotes whereas everything else should work without. In my opinion, it is often not obvious whether they are required or not which is why I tended to use more quotes than actually were needed. I am fairly familiar with quoting by now and can use both "quoting styles", but I think it would probably be a good idea to decide for one. Using just as much quotes as required would be a little bit shorter, but using quotes `where possible` may be a little bit more foolproof. $ a=" a d" $ b=" e" $ c=$a:$b $ echo "$c" a d: e $ $ $ filename="a d e" $ suffix=".f" $ [ -e ${filename}${suffix} ] && echo 1 bash: [: too many arguments $ [[ -e ${filename}${suffix} ]] && echo 1 $ touch "${filename}${suffix}" $ [[ -e ${filename}${suffix} ]] && echo 1 1 $ [[ -e a d e.f ]] && echo 1 bash: syntax error in conditional expression bash: syntax error near `d' $ [[ -e "a d e.f" ]] && echo 1 1
On Thu, Nov 12, 2009 at 11:52:31PM +0000, Cedric Staniewski wrote:
Allan McRae wrote:
Cedric Staniewski wrote:
We could remove the quotes here, too.
Hmmm... can we. I know we can when there are spaces in a single variable, but for some reason I thought quotes were needed when joining multiple variables like that. I could be wrong...
[[ behaves quite different compared to [ in this aspect, but as far as I know, you do not need quotes to join variables as long as there are no spaces between the variables ;). There are several exceptions though, like cd for example, which require quotes even for single variables when they contain spaces. I think this has something to do with the type of the command. Builtins/external commands (usually) need quotes whereas everything else should work without. In my opinion, it is often not obvious whether they are required or not which is why I tended to use more quotes than actually were needed. I am fairly familiar with quoting by now and can use both "quoting styles", but I think it would probably be a good idea to decide for one. Using just as much quotes as required would be a little bit shorter, but using quotes `where possible` may be a little bit more foolproof.
$ a=" a d" $ b=" e" $ c=$a:$b $ echo "$c" a d: e $ $ $ filename="a d e" $ suffix=".f" $ [ -e ${filename}${suffix} ] && echo 1 bash: [: too many arguments $ [[ -e ${filename}${suffix} ]] && echo 1 $ touch "${filename}${suffix}" $ [[ -e ${filename}${suffix} ]] && echo 1 1 $ [[ -e a d e.f ]] && echo 1 bash: syntax error in conditional expression bash: syntax error near `d' $ [[ -e "a d e.f" ]] && echo 1 1
The [[ ]] and (( )) are the exception to everything else in bash (cd, [, touch, grep). I know of nothing else that acts like it (except array indices). With all bash commands, the variable is expanded early enough that a space will split the result into multiple parameters (aka word splitting). Ergo these two are the same: $ cd a b $ var="a b"; cd $var cd will see two arguments. Compare to this, which has one parameter: $ cd "a b" $ var="a b"; cd "$var" The same applies to [, which can make stuff fun, eg: $ [ -n a -a -z a ] -> returns false because the second condition is false and there is an and (-a) $ var="a -a -z a"; [ -n $var ] -> false, too! Go figure... $ var="a -a -z a"; [ -n "$var" ] -> works right The [[ and (( are "special". Word splitting will not occur inside. (Array indices are computed in arithmetic mode, ie like inside ((: ${arr[stuff]}). The entire parsing is set up different. Word splitting does not occur. And (mostly as an aside,) bash parses them over double as fast. Concatenating variables has no special rules. It acts identical to one variable with both the contents. Assuming there is no whitespace between them, it is just like one variable. Whitespace inside causes word splitting but not in (( and [[. Once discussing word splitting, this also applies to arrays, specifically in this context: $ for var in ${arr[@]} ; do Without quotes, elements with a space will be turned into multiple elements for the loop. - Isaac
From f29298f8d9b947ce64d33d40d91f485396bb54eb Mon Sep 17 00:00:00 2001 From: Isaac Good <pacman@isaac.otherinbox.com> Date: Thu, 12 Nov 2009 15:07:34 -0500 Subject: [PATCH 1/2] Changing [ to [[ and (( - Part One - Amended
First half of makepkg This replaces any prior patches of mine Includes stuff like -o to || and -a to && etc if [ $(type .. were preserved due to a bash bug with [[ and set -e and ERR traps Signed-off-by: Isaac Good <pacman@isaac.otherinbox.com> --- scripts/makepkg.sh.in | 200 ++++++++++++++++++++++++------------------------ 1 files changed, 100 insertions(+), 100 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 25fb8d9..8082dfd 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -112,7 +112,7 @@ error() { # the fakeroot call, the error message will be printed by the main call. ## trap_exit() { - if [ "$INFAKEROOT" -eq 0 ]; then + if (( ! INFAKEROOT )); then echo error "$@" fi @@ -126,21 +126,21 @@ trap_exit() { clean_up() { local EXIT_CODE=$? - if [ "$INFAKEROOT" -eq 1 ]; then + if (( INFAKEROOT )); then # Don't clean up when leaving fakeroot, we're not done yet. return fi - if [ $EXIT_CODE -eq 0 -a "$CLEANUP" -eq 1 ]; then + if (( ! EXIT_CODE && CLEANUP )); then # If it's a clean exit and -c/--clean has been passed... msg "$(gettext "Cleaning up...")" rm -rf "$pkgdir" "$srcdir" - if [ -n "$pkgbase" ]; then + if [[ -n $pkgbase ]]; then # Can't do this unless the BUILDSCRIPT has been sourced. rm -f "${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-build.log"* - if [ "$PKGFUNC" -eq 1 ]; then + if (( PKGFUNC )); then rm -f "${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-package.log"* - elif [ "$SPLITPKG" -eq 1 ]; then + elif (( SPLITPKG )); then for pkg in ${pkgname[@]}; do rm -f "${pkg}-${pkgver}-${pkgrel}-${CARCH}-package.log"* done @@ -190,14 +190,14 @@ get_url() { ## check_option() { local ret=$(in_opt_array "$1" ${options[@]}) - if [ "$ret" != '?' ]; then + if [[ $ret != '?' ]]; then echo $ret return fi # fall back to makepkg.conf options ret=$(in_opt_array "$1" ${OPTIONS[@]}) - if [ "$ret" != '?' ]; then + if [[ $ret != '?' ]]; then echo $ret return fi @@ -231,10 +231,10 @@ in_opt_array() { local opt for opt in "$@"; do opt="${opt,,}" - if [ "$opt" = "$needle" ]; then + if [[ $opt = $needle ]]; then echo 'y' # Enabled return - elif [ "$opt" = "!$needle" ]; then + elif [[ $opt = "!$needle" ]]; then echo 'n' # Disabled return fi @@ -251,10 +251,10 @@ in_opt_array() { ## in_array() { local needle=$1; shift - [ -z "$1" ] && return 1 # Not Found + [[ -z $1 ]] && return 1 # Not Found local item for item in "$@"; do - [ "$item" = "$needle" ] && return 0 # Found + [[ $item = $needle ]] && return 0 # Found done return 1 # Not Found } @@ -268,14 +268,14 @@ get_downloadclient() { local i for i in "${DLAGENTS[@]}"; do local handler="${i%%::*}" - if [ "$proto" = "$handler" ]; then + if [[ $proto = $handler ]]; then agent="${i##*::}" break fi done # if we didn't find an agent, return an error - if [ -z "$agent" ]; then + if [[ -z $agent ]]; then error "$(gettext "There is no agent set up to handle %s URLs. Check %s.")" "$proto" "$MAKEPKG_CONF" plain "$(gettext "Aborting...")" exit 1 # $E_CONFIG_ERROR @@ -283,7 +283,7 @@ get_downloadclient() { # ensure specified program is installed local program="${agent%% *}" - if [ ! -x "$program" ]; then + if [[ ! -x $program ]]; then local baseprog=$(basename $program) error "$(gettext "The download program %s is not installed.")" "$baseprog" plain "$(gettext "Aborting...")" @@ -317,25 +317,25 @@ download_file() { local ret=0 eval "$dlcmd || ret=\$?" - if [ $ret -gt 0 ]; then - [ ! -s "$dlfile" ] && rm -f -- "$dlfile" + if (( ret )); then + [[ ! -s $dlfile ]] && rm -f -- "$dlfile" return $ret fi # rename the temporary download file to the final destination - if [ "$dlfile" != "$file" ]; then + if [[ $dlfile != $file ]]; then mv -f "$SRCDEST/$dlfile" "$SRCDEST/$file" fi } check_deps() { - [ $# -gt 0 ] || return + (( $# > 0 )) || return pmout=$(pacman $PACMAN_OPTS -T "$@") ret=$? - if [ $ret -eq 127 ]; then #unresolved deps + if (( ret == 127 )); then #unresolved deps echo "$pmout" - elif [ $ret -ne 0 ]; then + elif (( ret )); then error "$(gettext "Pacman returned a fatal error (%i): %s")" "$ret" "$pmout" exit 1 fi @@ -345,26 +345,26 @@ handle_deps() { local R_DEPS_SATISFIED=0 local R_DEPS_MISSING=1 - [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED local deplist="$*" - if [ "$DEP_BIN" -eq 0 ]; then + if (( ! DEP_BIN )); then return $R_DEPS_MISSING fi - if [ "$DEP_BIN" -eq 1 ]; then + if (( DEP_BIN )); then # install missing deps from binary packages (using pacman -S) msg "$(gettext "Installing missing dependencies...")" local ret=0 - if [ "$ASROOT" -eq 0 ]; then + if (( ! ASROOT )); then sudo pacman $PACMAN_OPTS -S --asdeps $deplist || ret=$? else pacman $PACMAN_OPTS -S --asdeps $deplist || ret=$? fi - if [ $ret -ne 0 ]; then + if (( ret )); then error "$(gettext "Pacman failed to install missing dependencies.")" exit 1 # TODO: error code fi @@ -385,7 +385,7 @@ resolve_deps() { local R_DEPS_MISSING=1 local deplist="$(check_deps $*)" - if [ -z "$deplist" ]; then + if [[ -z $deplist ]]; then return $R_DEPS_SATISFIED fi @@ -393,8 +393,8 @@ resolve_deps() { pkgdeps="$pkgdeps $deplist" # check deps again to make sure they were resolved deplist="$(check_deps $*)" - [ -z "$deplist" ] && return $R_DEPS_SATISFIED - elif [ "$DEP_BIN" -eq 1 ]; then + [[ -z $deplist ]] && return $R_DEPS_SATISFIED + elif (( DEP_BIN )); then error "$(gettext "Failed to install all missing dependencies.")" fi @@ -410,8 +410,8 @@ resolve_deps() { # fix flyspray bug #5923 remove_deps() { # $pkgdeps is a GLOBAL variable, set by resolve_deps() - [ "$RMDEPS" -eq 0 ] && return - [ -z "$pkgdeps" ] && return + (( ! RMDEPS )) && return + [[ -z $pkgdeps ]] && return local dep depstrip deplist deplist="" @@ -422,14 +422,14 @@ remove_deps() { msg "Removing installed dependencies..." local ret=0 - if [ "$ASROOT" -eq 0 ]; then + if (( ! ASROOT )); then sudo pacman $PACMAN_OPTS -Rns $deplist || ret=$? else pacman $PACMAN_OPTS -Rns $deplist || ret=$? fi # Fixes FS#10039 - exit cleanly as package has built successfully - if [ $ret -ne 0 ]; then + if (( ret )); then warning "$(gettext "Failed to remove installed dependencies.")" return 0 fi @@ -438,7 +438,7 @@ remove_deps() { download_sources() { msg "$(gettext "Retrieving Sources...")" - if [ ! -w "$SRCDEST" ] ; then + if [[ ! -w $SRCDEST ]] ; then error "$(gettext "You do not have write permission to store downloads in %s.")" "$SRCDEST" plain "$(gettext "Aborting...")" exit 1 @@ -450,12 +450,12 @@ download_sources() { for netfile in "${source[@]}"; do local file=$(get_filename "$netfile") local url=$(get_url "$netfile") - if [ -f "$startdir/$file" ]; then + if [[ -f $startdir/$file ]]; then msg2 "$(gettext "Found %s in build dir")" "$file" rm -f "$srcdir/$file" ln -s "$startdir/$file" "$srcdir/" continue - elif [ -f "$SRCDEST/$file" ]; then + elif [[ -f $SRCDEST/$file ]]; then msg2 "$(gettext "Using cached copy of %s")" "$file" rm -f "$srcdir/$file" ln -s "$SRCDEST/$file" "$srcdir/" @@ -463,7 +463,7 @@ download_sources() { fi # if we get here, check to make sure it was a URL, else fail - if [ "$file" = "$url" ]; then + if [[ $file = $url ]]; then error "$(gettext "%s was not found in the build directory and is not a URL.")" "$file" exit 1 # $E_MISSING_FILE fi @@ -475,7 +475,7 @@ download_sources() { # fix flyspray bug #3289 local ret=0 download_file "$dlclient" "$url" "$file" || ret=$? - if [ $ret -gt 0 ]; then + if (( ret )); then error "$(gettext "Failure while downloading %s")" "$file" plain "$(gettext "Aborting...")" exit 1 @@ -521,8 +521,8 @@ generate_checksums() { for netfile in "${source[@]}"; do local file="$(get_filename "$netfile")" - if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f $SRCDEST/$file ]] ; then error "$(gettext "Unable to find source file %s to generate checksum.")" "$file" plain "$(gettext "Aborting...")" exit 1 @@ -533,10 +533,10 @@ generate_checksums() { local sum="$(openssl dgst -${integ} "$file")" sum=${sum##* } - [ $ct -gt 0 ] && echo -n "$indent" + (( ct )) && echo -n "$indent" echo -n "'$sum'" ct=$(($ct+1)) - [ $ct -lt $numsrc ] && echo + (( $ct < $numsrc )) && echo done echo ")" @@ -544,7 +544,7 @@ generate_checksums() { } check_checksums() { - [ ${#source[@]} -eq 0 ] && return 0 + (( ! ${#source[@]} )) && return 0 if [ ! $(type -p openssl) ]; then error "$(gettext "Cannot find openssl.")" @@ -555,7 +555,7 @@ check_checksums() { local integ required for integ in md5 sha1 sha256 sha384 sha512; do local integrity_sums=($(eval echo "\${${integ}sums[@]}")) - if [ ${#integrity_sums[@]} -eq ${#source[@]} ]; then + if (( ${#integrity_sums[@]} == ${#source[@]} )); then msg "$(gettext "Validating source files with %s...")" "${integ}sums" correlation=1 local errors=0 @@ -566,8 +566,8 @@ check_checksums() { file="$(get_filename "$file")" echo -n " $file ... " >&2 - if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f $SRCDEST/$file ]] ; then echo "$(gettext "NOT FOUND")" >&2 errors=1 found=0 @@ -576,11 +576,11 @@ check_checksums() { fi fi - if [ $found -gt 0 ] ; then + if (( $found )) ; then local expectedsum="${integrity_sums[$idx],,}" local realsum="$(openssl dgst -${integ} "$file")" realsum="${realsum##* }" - if [ "$expectedsum" = "$realsum" ]; then + if [[ $expectedsum = $realsum ]]; then echo "$(gettext "Passed")" >&2 else echo "$(gettext "FAILED")" >&2 @@ -591,18 +591,18 @@ check_checksums() { idx=$((idx + 1)) done - if [ $errors -gt 0 ]; then + if (( errors )); then error "$(gettext "One or more files did not pass the validity check!")" exit 1 # TODO: error code fi - elif [ ${#integrity_sums[@]} -gt 0 ]; then + elif (( ${#integrity_sums[@]} )); then error "$(gettext "Integrity checks (%s) differ in size from the source array.")" "$integ" exit 1 # TODO: error code fi done - if [ $correlation -eq 0 ]; then - if [ $SKIPINTEG -eq 1 ]; then + if (( ! correlation )); then + if (( SKIPINTEG )); then warning "$(gettext "Integrity checks are missing.")" else error "$(gettext "Integrity checks are missing.")" @@ -622,8 +622,8 @@ extract_sources() { continue fi - if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f $SRCDEST/$file ]] ; then error "$(gettext "Unable to find source file %s for extraction.")" "$file" plain "$(gettext "Aborting...")" exit 1 @@ -662,31 +662,31 @@ extract_sources() { local ret=0 msg2 "$(gettext "Extracting %s with %s")" "$file" "$cmd" - if [ "$cmd" = "bsdtar" ]; then + if [[ $cmd = bsdtar ]]; then $cmd -xf "$file" || ret=? else rm -f "${file%.*}" $cmd -dcf "$file" > "${file%.*}" || ret=? fi - if [ $ret -ne 0 ]; then + if (( ret )); then error "$(gettext "Failed to extract %s")" "$file" plain "$(gettext "Aborting...")" exit 1 fi done - if [ $EUID -eq 0 ]; then + if (( EUID == 0 )); then # change perms of all source files to root user & root group chown -R 0:0 "$srcdir" fi } error_function() { - if [ -p "$logpipe" ]; then + if [[ -p $logpipe ]]; then rm "$logpipe" fi # first exit all subshells, then print the error - if [ $BASH_SUBSHELL -eq 0 ]; then + if (( ! BASH_SUBSHELL )); then plain "$(gettext "Aborting...")" remove_deps fi @@ -694,13 +694,13 @@ error_function() { } run_function() { - if [ -z "$1" ]; then + if [[ -z $1 ]]; then return 1 fi pkgfunc="$1" # clear user-specified makeflags if requested - if [ "$(check_option makeflags)" = "n" ]; then + if [[ $(check_option makeflags) = "n" ]]; then MAKEFLAGS="" fi @@ -713,12 +713,12 @@ run_function() { local shellopts=$(shopt -p) local ret=0 - if [ "$LOGGING" -eq 1 ]; then + if (( LOGGING )); then BUILDLOG="${startdir}/${pkgname}-${pkgver}-${pkgrel}-${CARCH}-$pkgfunc.log" - if [ -f "$BUILDLOG" ]; then + if [[ -f $BUILDLOG ]]; then local i=1 while true; do - if [ -f "$BUILDLOG.$i" ]; then + if [[ -f $BUILDLOG.$i ]]; then i=$(($i +1)) else break @@ -752,24 +752,24 @@ run_function() { run_build() { # use distcc if it is requested (check buildenv and PKGBUILD opts) - if [ "$(check_buildenv distcc)" = "y" -a "$(check_option distcc)" != "n" ]; then - [ -d /usr/lib/distcc/bin ] && export PATH="/usr/lib/distcc/bin:$PATH" + if [[ $(check_buildenv distcc) = "y" && $(check_option distcc) != "n" ]]; then + [[ -d /usr/lib/distcc/bin ]] && export PATH="/usr/lib/distcc/bin:$PATH" export DISTCC_HOSTS - elif [ "$(check_option distcc)" = "n" ]; then + elif [[ $(check_option distcc) = "n" ]]; then # if it is not wanted, clear the makeflags too MAKEFLAGS="" fi # use ccache if it is requested (check buildenv and PKGBUILD opts) - if [ "$(check_buildenv ccache)" = "y" -a "$(check_option ccache)" != "n" ]; then - [ -d /usr/lib/ccache/bin ] && export PATH="/usr/lib/ccache/bin:$PATH" + if [[ $(check_buildenv ccache) = "y" && $(check_option ccache) != "n" ]]; then + [[ -d /usr/lib/ccache/bin ]] && export PATH="/usr/lib/ccache/bin:$PATH" fi run_function "build" } run_package() { - if [ -z "$1" ]; then + if [[ -z $1 ]]; then pkgfunc="package" else pkgfunc="package_$1" @@ -782,16 +782,16 @@ tidy_install() { cd "$pkgdir" msg "$(gettext "Tidying install...")" - if [ "$(check_option docs)" = "n" -a -n "${DOC_DIRS[*]}" ]; then + if [[ $(check_option docs) = "n" && -n ${DOC_DIRS[*]} ]]; then msg2 "$(gettext "Removing doc files...")" rm -rf ${DOC_DIRS[@]} fi - if [ "$(check_option purge)" = "y" -a -n "${PURGE_TARGETS[*]}" ]; then + if [[ $(check_option purge) = "y" && -n ${PURGE_TARGETS[*]} ]]; then msg2 "$(gettext "Purging other files...")" local pt for pt in "${PURGE_TARGETS[@]}"; do - if [ "${pt}" = "${pt//\/}" ]; then + if [[ ${pt} = ${pt//\/} ]]; then find . -type f -name "${pt}" -exec rm -f -- '{}' \; else rm -f ${pt} @@ -799,16 +799,16 @@ tidy_install() { done fi - if [ "$(check_option zipman)" = "y" -a -n "${MAN_DIRS[*]}" ]; then + if [[ $(check_option zipman) = "y" && -n ${MAN_DIRS[*]} ]]; then msg2 "$(gettext "Compressing man and info pages...")" local manpage ext file link hardlinks hl find ${MAN_DIRS[@]} -type f 2>/dev/null | while read manpage ; do # check file still exists (potentially compressed with hard link) - if [ -f ${manpage} ]; then + if [[ -f ${manpage} ]]; then ext="${manpage##*.}" file="${manpage##*/}" - if [ "$ext" != "gz" -a "$ext" != "bz2" ]; then + if [[ $ext != gz && $ext != bz2 ]]; then # update symlinks to this manpage find ${MAN_DIRS[@]} -lname "$file" 2>/dev/null | while read link ; do @@ -834,7 +834,7 @@ tidy_install() { done fi - if [ "$(check_option strip)" = "y" -a -n "${STRIP_DIRS[*]}" ]; then + if [[ $(check_option strip) = y && -n ${STRIP_DIRS[*]} ]]; then msg2 "$(gettext "Stripping debugging symbols from binaries and libraries...")" local binary find ${STRIP_DIRS[@]} -type f 2>/dev/null | while read binary ; do @@ -851,12 +851,12 @@ tidy_install() { done fi - if [ "$(check_option libtool)" = "n" ]; then + if [[ $(check_option libtool) = "n" ]]; then msg2 "$(gettext "Removing libtool .la files...")" find . ! -type d -name "*.la" -exec rm -f -- '{}' \; fi - if [ "$(check_option emptydirs)" = "n" ]; then + if [[ $(check_option emptydirs) = "n" ]]; then msg2 "$(gettext "Removing empty directories...")" find . -depth -type d -empty -delete fi @@ -864,7 +864,7 @@ tidy_install() { write_pkginfo() { local builddate=$(date -u "+%s") - if [ -n "$PACKAGER" ]; then + if [[ -n $PACKAGER ]]; then local packager="$PACKAGER" else local packager="Unknown Packager" @@ -874,12 +874,12 @@ write_pkginfo() { msg2 "$(gettext "Generating .PKGINFO file...")" echo "# Generated by makepkg $myver" >.PKGINFO - if [ "$INFAKEROOT" -eq 1 ]; then + if (( INFAKEROOT )); then echo "# using $(fakeroot -v)" >>.PKGINFO fi echo "# $(LC_ALL=C date -u)" >>.PKGINFO echo "pkgname = $1" >>.PKGINFO - [ "$SPLITPKG" -eq 1 ] && echo "pkgbase = $pkgbase" >>.PKGINFO + (( SPLITPKG )) && echo pkgbase = $pkgbase >>.PKGINFO echo "pkgver = $pkgver-$pkgrel" >>.PKGINFO echo "pkgdesc = $pkgdesc" >>.PKGINFO echo "url = $url" >>.PKGINFO @@ -887,7 +887,7 @@ write_pkginfo() { echo "packager = $packager" >>.PKGINFO echo "size = $size" >>.PKGINFO echo "arch = $PKGARCH" >>.PKGINFO - if [ "$(check_option force)" = "y" ]; then + if [[ $(check_option force) = "y" ]]; then echo "force = true" >> .PKGINFO fi @@ -918,8 +918,8 @@ write_pkginfo() { done for it in "${packaging_options[@]}"; do local ret="$(check_option $it)" - if [ "$ret" != "?" ]; then - if [ "$ret" = "y" ]; then + if [[ $ret != "?" ]]; then + if [[ $ret = y ]]; then echo "makepkgopt = $it" >>.PKGINFO else echo "makepkgopt = !$it" >>.PKGINFO @@ -929,7 +929,7 @@ write_pkginfo() { # TODO maybe remove this at some point # warn if license array is not present or empty - if [ -z "$license" ]; then + if [[ -z $license ]]; then warning "$(gettext "Please add a license line to your %s!")" "$BUILDSCRIPT" plain "$(gettext "Example for GPL\'ed software: license=('GPL').")" fi @@ -941,14 +941,14 @@ check_package() { # check existence of backup files local file for file in "${backup[@]}"; do - if [ ! -f "$file" ]; then + if [[ ! -f $file ]]; then warning "$(gettext "Invalid backup entry : %s")" "$file" fi done } create_package() { - if [ ! -d "$pkgdir" ]; then + if [[ ! -d $pkgdir ]]; then error "$(gettext "Missing pkg/ directory.")" plain "$(gettext "Aborting...")" exit 1 # $E_MISSING_PKGDIR @@ -959,13 +959,13 @@ create_package() { cd "$pkgdir" msg "$(gettext "Creating package...")" - if [ -z "$1" ]; then + if [[ -z $1 ]]; then nameofpkg="$pkgname" else nameofpkg="$1" fi - if [ "$arch" = "any" ]; then + if [[ $arch = "any" ]]; then PKGARCH="any" else PKGARCH=$CARCH @@ -976,14 +976,14 @@ create_package() { local comp_files=".PKGINFO" # check for an install script - if [ -n "$install" ]; then + if [[ -n $install ]]; then msg2 "$(gettext "Adding install script...")" cp "$startdir/$install" .INSTALL comp_files="$comp_files .INSTALL" fi # do we have a changelog? - if [ -n "$changelog" ]; then + if [[ -n $changelog ]]; then msg2 "$(gettext "Adding package changelog...")" cp "$startdir/$changelog" .CHANGELOG comp_files="$comp_files .CHANGELOG" @@ -1009,7 +1009,7 @@ create_package() { bsdtar -cf - $comp_files * > "$pkg_file" || ret=$? shopt -u nullglob - if [ $ret -eq 0 ]; then + if (( ! ret )); then case "$PKGEXT" in *tar.gz) gzip -f -n "$pkg_file" ;; *tar.bz2) bzip2 -f "$pkg_file" ;; @@ -1018,7 +1018,7 @@ create_package() { ret=$? fi - if [ $ret -ne 0 ]; then + if (( ret )); then error "$(gettext "Failed to create package file.")" exit 1 # TODO: error code fi @@ -1042,8 +1042,8 @@ create_srcpackage() { msg2 "$(gettext "Adding %s...")" "$BUILDSCRIPT" ln -s "${BUILDFILE}" "${srclinks}/${pkgbase}/${BUILDSCRIPT}" - if [ -n "$install" ]; then - if [ -f $install ]; then + if [[ -n $install ]]; then + if [[ -f $install ]]; then msg2 "$(gettext "Adding install script...")" ln -s "${startdir}/$install" "${srclinks}/${pkgbase}/" else @@ -1051,8 +1051,8 @@ create_srcpackage() { fi fi - if [ -n "$changelog" ]; then - if [ -f "$changelog" ]; then + if [[ -n $changelog ]]; then + if [[ -f $changelog ]]; then msg2 "$(gettext "Adding package changelog...")" ln -s "${startdir}/$changelog" "${srclinks}/${pkgbase}/" else @@ -1063,10 +1063,10 @@ create_srcpackage() { local netfile for netfile in "${source[@]}"; do local file=$(get_filename "$netfile") - if [ -f "$netfile" ]; then + if [[ -f $netfile ]]; then msg2 "$(gettext "Adding %s...")" "$netfile" ln -s "${startdir}/$netfile" "${srclinks}/${pkgbase}" - elif [ "$SOURCEONLY" -eq 2 -a -f "$SRCDEST/$file" ]; then + elif (( SOURCEONLY == 2 )) && [[ -f $SRCDEST/$file ]]; then msg2 "$(gettext "Adding %s...")" "$file" ln -s "$SRCDEST/$file" "${srclinks}/${pkgbase}/" fi -- 1.6.5.2
From e3e5bb93cb2b33315a8d6eff8956aa5b6c74d28d Mon Sep 17 00:00:00 2001 From: Isaac Good <pacman@isaac.otherinbox.com> Date: Thu, 12 Nov 2009 15:09:05 -0500 Subject: [PATCH 2/2] Changing [ to [[ and (( - Part Two - Amended
Second half of makepkg This replaces any prior patches of mine Includes stuff like -o to || and -a to && etc if [ $(type .. were preserved due to a bash bug with [[ and set -e and ERR trap Signed-off-by: Isaac Good <pacman@isaac.otherinbox.com> --- scripts/makepkg.sh.in | 216 ++++++++++++++++++++++++------------------------- 1 files changed, 107 insertions(+), 109 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8082dfd..ed05e7e 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1095,9 +1095,9 @@ create_srcpackage() { } install_package() { - [ "$INSTALL" -eq 0 ] && return + (( ! INSTALL )) && return - if [ "$SPLITPKG" -eq 0 ]; then + if (( ! SPLITPKG )); then msg "$(gettext "Installing package ${pkgname} with pacman -U...")" else msg "$(gettext "Installing ${pkgbase} package group with pacman -U...")" @@ -1105,7 +1105,7 @@ install_package() { local pkglist for pkg in ${pkgname[@]}; do - if [ -f $PKGDEST/${pkg}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT} ]; then + if [[ -f $PKGDEST/${pkg}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT} ]]; then pkglist="${pkglist} $PKGDEST/${pkg}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT}" else pkglist="${pkglist} $PKGDEST/${pkg}-${pkgver}-${pkgrel}-any${PKGEXT}" @@ -1113,13 +1113,13 @@ install_package() { done local ret=0 - if [ "$ASROOT" -eq 0 ]; then + if (( ! ASROOT )); then sudo pacman $PACMAN_OPTS -U ${pkglist} || ret=$? else pacman $PACMAN_OPTS -U ${pkglist} || ret=$? fi - if [ $ret -ne 0 ]; then + if (( ret )); then warning "$(gettext "Failed to install built package(s).")" return 0 fi @@ -1127,34 +1127,34 @@ install_package() { check_sanity() { # check for no-no's in the build script - if [ -z "$pkgname" ]; then + if [[ -z $pkgname ]]; then error "$(gettext "%s is not allowed to be empty.")" "pkgname" return 1 fi - if [ -z "$pkgver" ]; then + if [[ -z $pkgver ]]; then error "$(gettext "%s is not allowed to be empty.")" "pkgver" return 1 fi - if [ -z "$pkgrel" ]; then + if [[ -z $pkgrel ]]; then error "$(gettext "%s is not allowed to be empty.")" "pkgrel" return 1 fi - if [ "${pkgname:0:1}" == "-" ]; then + if [[ ${pkgname:0:1} == "-" ]]; then error "$(gettext "%s is not allowed to start with a hyphen.")" "pkgname" return 1 fi - if [ "$pkgver" != "${pkgver//-/}" ]; then + if [[ $pkgver != ${pkgver//-/} ]]; then error "$(gettext "%s is not allowed to contain hyphens.")" "pkgver" return 1 fi - if [ "$pkgrel" != "${pkgrel//-/}" ]; then + if [[ $pkgrel != ${pkgrel//-/} ]]; then error "$(gettext "%s is not allowed to contain hyphens.")" "pkgrel" return 1 fi - if [ "$arch" != 'any' ]; then + if [[ $arch != 'any' ]]; then if ! in_array $CARCH ${arch[@]}; then - if [ "$IGNOREARCH" -eq 0 ]; then + if (( ! IGNOREARCH )); then error "$(gettext "%s is not available for the '%s' architecture.")" "$pkgbase" "$CARCH" plain "$(gettext "Note that many packages may need a line added to their %s")" "$BUILDSCRIPT" plain "$(gettext "such as arch=('%s').")" "$CARCH" @@ -1165,7 +1165,7 @@ check_sanity() { local provide for provide in ${provides[@]}; do - if [ $provide != ${provide//</} -o $provide != ${provide//>/} ]; then + if [[ $provide != ${provide//</} || $provide != ${provide//>/} ]]; then error "$(gettext "Provides array cannot contain comparison (< or >) operators.")" return 1 fi @@ -1173,7 +1173,7 @@ check_sanity() { local file for file in "${backup[@]}"; do - if [ "${file:0:1}" = "/" ]; then + if [[ ${file:0:1} = "/" ]]; then error "$(gettext "Invalid backup entry : %s")" "$file" return 1 fi @@ -1187,12 +1187,12 @@ check_sanity() { fi done - if [ "$install" -a ! -f "$install" ]; then + if [[ $install && ! -f $install ]]; then error "$(gettext "Install scriptlet (%s) does not exist.")" "$install" return 1 fi - if [ -n "$changelog" -a ! -f "$changelog" ]; then + if [[ -n $changelog && ! -f $changelog ]]; then error "$(gettext "Changelog file (%s) does not exist.")" "$changelog" return 1 fi @@ -1203,20 +1203,20 @@ check_sanity() { known=0 # check if option matches a known option or its inverse for kopt in ${packaging_options[@]} ${other_options[@]}; do - if [ "${opt}" = "${kopt}" -o "${opt}" = "!${kopt}" ]; then + if [[ ${opt} = ${kopt} || ${opt} = "!${kopt}" ]]; then known=1 fi done - if [ $known -eq 0 ]; then + if (( ! known )); then error "$(gettext "options array contains unknown option '%s'")" "$opt" valid_options=0 fi done - if [ $valid_options -eq 0 ]; then + if (( ! valid_options )); then return 1 fi - if [ "${#pkgname[@]}" -gt "1" ]; then + if (( ${#pkgname[@]} > 1 )); then for pkg in ${pkgname[@]}; do if [ "$(type -t package_${pkg})" != "function" ]; then error "$(gettext "missing package function for split package '%s'")" "$pkg" @@ -1233,42 +1233,42 @@ devel_check() { # Do not update pkgver if --holdver is set, when building a source package, # when reading PKGBUILD from pipe (-f), or if we cannot write to the file (-w) - if [ "$HOLDVER" -eq 1 -o "$SOURCEONLY" -ne 0 -o ! -f "$BUILDFILE" \ - -o ! -w "$BUILDFILE" ]; then + if (( HOLDVER || SOURCEONLY )) \ + || [[ ! -f $BUILDFILE || ! -w $BUILDFILE ]]; then return fi - if [ -z "$FORCE_VER" ]; then + if [[ -z $FORCE_VER ]]; then # Check if this is a svn/cvs/etc PKGBUILD; set $newpkgver if so. # This will only be used on the first call to makepkg; subsequent # calls to makepkg via fakeroot will explicitly pass the version # number to avoid having to determine the version number twice. # Also do a brief check to make sure we have the VCS tool available. oldpkgver=$pkgver - if [ -n "${_darcstrunk}" -a -n "${_darcsmod}" ] ; then + if [[ -n ${_darcstrunk} && -n ${_darcsmod} ]] ; then [ $(type -p darcs) ] || return 0 msg "$(gettext "Determining latest darcs revision...")" newpkgver=$(date +%Y%m%d) - elif [ -n "${_cvsroot}" -a -n "${_cvsmod}" ] ; then + elif [[ -n ${_cvsroot} && -n ${_cvsmod} ]] ; then [ $(type -p cvs) ] || return 0 msg "$(gettext "Determining latest cvs revision...")" newpkgver=$(date +%Y%m%d) - elif [ -n "${_gitroot}" -a -n "${_gitname}" ] ; then + elif [[ -n ${_gitroot} && -n ${_gitname} ]] ; then [ $(type -p git) ] || return 0 msg "$(gettext "Determining latest git revision...")" newpkgver=$(date +%Y%m%d) - elif [ -n "${_svntrunk}" -a -n "${_svnmod}" ] ; then + elif [[ -n ${_svntrunk} && -n ${_svnmod} ]] ; then [ $(type -p svn) ] || return 0 msg "$(gettext "Determining latest svn revision...")" newpkgver=$(LC_ALL=C svn info $_svntrunk | sed -n 's/^Last Changed Rev: \([0-9]*\)$/\1/p') - elif [ -n "${_bzrtrunk}" -a -n "${_bzrmod}" ] ; then + elif [[ -n ${_bzrtrunk} && -n ${_bzrmod} ]] ; then [ $(type -p bzr) ] || return 0 msg "$(gettext "Determining latest bzr revision...")" newpkgver=$(bzr revno ${_bzrtrunk}) - elif [ -n "${_hgroot}" -a -n "${_hgrepo}" ] ; then + elif [[ -n ${_hgroot} && -n ${_hgrepo} ]] ; then [ $(type -p hg) ] || return 0 msg "$(gettext "Determining latest hg revision...")" - if [ -d ./src/$_hgrepo ] ; then + if [[ -d ./src/$_hgrepo ]] ; then cd ./src/$_hgrepo hg pull hg update @@ -1281,7 +1281,7 @@ devel_check() { cd ../../ fi - if [ -n "$newpkgver" ]; then + if [[ -n $newpkgver ]]; then msg2 "$(gettext "Version found: %s")" "$newpkgver" fi @@ -1301,9 +1301,9 @@ devel_update() { # ... # _foo=pkgver # - if [ -n "$newpkgver" ]; then - if [ "$newpkgver" != "$pkgver" ]; then - if [ -f "$BUILDFILE" -a -w "$BUILDFILE" ]; then + if [[ -n $newpkgver ]]; then + if [[ $newpkgver != $pkgver ]]; then + if [[ -f $BUILDFILE && -w $BUILDFILE ]]; then @SEDINPLACE@ "s/^pkgver=[^ ]*/pkgver=$newpkgver/" "$BUILDFILE" @SEDINPLACE@ "s/^pkgrel=[^ ]*/pkgrel=1/" "$BUILDFILE" source "$BUILDFILE" @@ -1322,7 +1322,7 @@ backup_package_variables() { restore_package_variables() { for var in ${splitpkg_overrides[@]}; do indirect="${var}_backup" - if [ -n "${!indirect}" ]; then + if [[ -n ${!indirect} ]]; then eval "${var}=(\"\${$indirect[@]}\")" else unset ${var} @@ -1337,21 +1337,21 @@ parse_options() { local ret=0; local unused_options="" - while [ -n "$1" ]; do - if [ ${1:0:2} = '--' ]; then - if [ -n "${1:2}" ]; then + while [[ -n $1 ]]; do + if [[ ${1:0:2} = '--' ]]; then + if [[ -n ${1:2} ]]; then local match="" for i in ${long_options//,/ }; do - if [ ${1:2} = ${i//:} ]; then + if [[ ${1:2} = ${i//:} ]]; then match=$i break fi done - if [ -n "$match" ]; then - if [ ${1:2} = $match ]; then + if [[ -n $match ]]; then + if [[ ${1:2} = $match ]]; then printf ' %s' "$1" else - if [ -n "$2" ]; then + if [[ -n $2 ]]; then printf ' %s' "$1" shift printf " '%s'" "$1" @@ -1368,15 +1368,15 @@ parse_options() { shift break fi - elif [ ${1:0:1} = '-' ]; then + elif [[ ${1:0:1} = '-' ]]; then for ((i=1; i<${#1}; i++)); do - if [[ "$short_options" =~ "${1:i:1}" ]]; then - if [[ "$short_options" =~ "${1:i:1}:" ]]; then - if [ -n "${1:$i+1}" ]; then + if [[ $short_options =~ ${1:i:1} ]]; then + if [[ $short_options =~ "${1:i:1}:" ]]; then + if [[ -n ${1:$i+1} ]]; then printf ' -%s' "${1:i:1}" printf " '%s'" "${1:$i+1}" else - if [ -n "$2" ]; then + if [[ -n $2 ]]; then printf ' -%s' "${1:i:1}" shift printf " '%s'" "${1}" @@ -1401,13 +1401,13 @@ parse_options() { done printf " --" - if [ -n "$unused_options" ]; then + if [[ -n $unused_options ]]; then for i in ${unused_options[@]}; do printf ' %s' "$i" done fi - if [ -n "$1" ]; then - while [ -n "$1" ]; do + if [[ -n $1 ]]; then + while [[ -n $1 ]]; do printf " '%s'" "${1}" shift done @@ -1540,7 +1540,7 @@ _SRCDEST=${SRCDEST} MAKEPKG_CONF=${MAKEPKG_CONF:-$confdir/makepkg.conf} # Source the config file; fail if it is not found -if [ -r "$MAKEPKG_CONF" ]; then +if [[ -r $MAKEPKG_CONF ]]; then source "$MAKEPKG_CONF" else error "$(gettext "%s not found.")" "$MAKEPKG_CONF" @@ -1549,13 +1549,13 @@ else fi # Source user-specific makepkg.conf overrides -if [ -r ~/.makepkg.conf ]; then +if [[ -r ~/.makepkg.conf ]]; then source ~/.makepkg.conf fi # check if messages are to be printed using color unset ALL_OFF BOLD BLUE GREEN RED YELLOW -if [ -t 2 -a ! "$USE_COLOR" = "n" -a "$(check_buildenv color)" = "y" ]; then +if [[ -t 2 && ! $USE_COLOR = "n" && $(check_buildenv color) = "y" ]]; then ALL_OFF="$(tput sgr0)" BOLD="$(tput bold)" BLUE="${BOLD}$(tput setaf 4)" @@ -1572,23 +1572,23 @@ SRCDEST=${_SRCDEST:-$SRCDEST} SRCDEST=${SRCDEST:-$startdir} #default to $startdir if undefined -if [ "$HOLDVER" -eq 1 -a -n "$FORCE_VER" ]; then +if (( HOLDVER )) && [[ -n $FORCE_VER ]]; then # The '\\0' is here to prevent gettext from thinking --holdver is an option error "$(gettext "\\0--holdver and --forcever cannot both be specified" )" exit 1 fi -if [ "$CLEANCACHE" -eq 1 ]; then +if (( CLEANCACHE )); then #fix flyspray feature request #5223 - if [ -n "$SRCDEST" -a "$SRCDEST" != "$startdir" ]; then + if [[ -n $SRCDEST && $SRCDEST != $startdir ]]; then msg "$(gettext "Cleaning up ALL files from %s.")" "$SRCDEST" echo -n "$(gettext " Are you sure you wish to do this? ")" echo -n "$(gettext "[y/N]")" read answer answer="${answer^^}" - if [ "$answer" = "$(gettext "YES")" -o "$answer" = "$(gettext "Y")" ]; then + if [[ $answer = $(gettext YES) || $answer = $(gettext Y) ]]; then rm "$SRCDEST"/* - if [ $? -ne 0 ]; then + if (( $? )); then error "$(gettext "Problem removing files; you may not have correct permissions in %s")" "$SRCDEST" exit 1 else @@ -1609,40 +1609,39 @@ if [ "$CLEANCACHE" -eq 1 ]; then fi fi -if [ "$INFAKEROOT" -eq 0 ]; then - if [ $EUID -eq 0 -a "$ASROOT" -eq 0 ]; then +if (( ! INFAKEROOT )); then + if (( EUID == 0 && ! ASROOT )); then # Warn those who like to live dangerously. error "$(gettext "Running makepkg as root is a BAD idea and can cause")" plain "$(gettext "permanent, catastrophic damage to your system. If you")" plain "$(gettext "wish to run as root, please use the --asroot option.")" exit 1 # $E_USER_ABORT - elif [ $EUID -gt 0 -a "$ASROOT" -eq 1 ]; then + elif (( EUID > 0 && ASROOT )); then # Warn those who try to use the --asroot option when they are not root error "$(gettext "The --asroot option is meant for the root user only.")" plain "$(gettext "Please rerun makepkg without the --asroot flag.")" exit 1 # $E_USER_ABORT - elif [ "$(check_buildenv fakeroot)" = "y" -a $EUID -gt 0 ]; then + elif [[ $(check_buildenv fakeroot) = "y" ]] && (( EUID > 0 )); then if [ ! $(type -p fakeroot) ]; then error "$(gettext "Fakeroot must be installed if using the 'fakeroot' option")" plain "$(gettext "in the BUILDENV array in %s.")" "$MAKEPKG_CONF" exit 1 fi - elif [ $EUID -gt 0 ]; then + elif (( EUID > 0 )); then warning "$(gettext "Running makepkg as an unprivileged user will result in non-root")" plain "$(gettext "ownership of the packaged files. Try using the fakeroot environment by")" plain "$(gettext "placing 'fakeroot' in the BUILDENV array in %s.")" "$MAKEPKG_CONF" sleep 1 fi else - if [ -z "$FAKEROOTKEY" ]; then + if [[ -z $FAKEROOTKEY ]]; then error "$(gettext "Do not use the '-F' option. This option is only for use by makepkg.")" exit 1 # TODO: error code fi fi # check for sudo if we will need it during makepkg execution -if [ "$ASROOT" -eq 0 \ - -a \( "$DEP_BIN" -eq 1 -o "$RMDEPS" -eq 1 -o "$INSTALL" -eq 1 \) ]; then +if (( ! ASROOT && ( DEP_BIN || RMDEPS || INSTALL ) )); then if [ ! "$(type -p sudo)" ]; then error "$(gettext "Cannot find the sudo binary! Is sudo installed?")" plain "$(gettext "Missing dependencies cannot be installed or removed as a normal user")" @@ -1656,8 +1655,8 @@ unset md5sums replaces depends conflicts backup source install changelog build unset makedepends optdepends options noextract BUILDFILE=${BUILDFILE:-$BUILDSCRIPT} -if [ ! -f "$BUILDFILE" ]; then - if [ -t 0 ]; then +if [[ ! -f $BUILDFILE ]]; then + if [[ -t 0 ]]; then error "$(gettext "%s does not exist.")" "$BUILDFILE" exit 1 else @@ -1667,18 +1666,18 @@ if [ ! -f "$BUILDFILE" ]; then fi else crlftest=$(file "$BUILDFILE" | grep -F 'CRLF' || true) - if [ -n "$crlftest" ]; then + if [[ -n $crlftest ]]; then error "$(gettext "%s contains CRLF characters and cannot be sourced.")" "$BUILDFILE" exit 1 fi - if [ "${BUILDFILE:0:1}" != "/" ]; then + if [[ ${BUILDFILE:0:1} != "/" ]]; then BUILDFILE="$startdir/$BUILDFILE" fi source "$BUILDFILE" fi -if [ "$GENINTEG" -eq 1 ]; then +if (( GENINTEG )); then mkdir -p "$srcdir" cd "$srcdir" download_sources @@ -1700,17 +1699,17 @@ check_sanity || exit 1 devel_check devel_update -if [ "${#pkgname[@]}" -gt "1" ]; then +if (( ${#pkgname[@]} > 1 )); then SPLITPKG=1 fi pkgbase=${pkgbase:-${pkgname[0]}} -if [ "$SPLITPKG" -eq 0 ]; then - if [ \( -f "$PKGDEST/${pkgname}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT}" \ - -o -f "$PKGDEST/${pkgname}-${pkgver}-${pkgrel}-any${PKGEXT}" \) \ - -a "$FORCE" -eq 0 -a "$SOURCEONLY" -eq 0 -a "$NOBUILD" -eq 0 ]; then - if [ "$INSTALL" -eq 1 ]; then +if (( ! SPLITPKG )); then + if [[ -f $PKGDEST/${pkgname}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT} \ + || -f $PKGDEST/${pkgname}-${pkgver}-${pkgrel}-any${PKGEXT} ]] \ + && ! (( FORCE || SOURCEONLY || NOBUILD )); then + if (( INSTALL )); then warning "$(gettext "A package has already been built, installing existing package...")" install_package exit $? @@ -1723,16 +1722,16 @@ else allpkgbuilt=1 somepkgbuilt=0 for pkg in ${pkgname[@]}; do - if [ \( -f "$PKGDEST/${pkg}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT}" \ - -o -f "$PKGDEST/${pkg}-${pkgver}-${pkgrel}-any${PKGEXT}" \) ]; then + if [[ -f $PKGDEST/${pkg}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT} \ + || -f $PKGDEST/${pkg}-${pkgver}-${pkgrel}-any${PKGEXT} ]]; then somepkgbuilt=1 else allpkgbuilt=0 fi done - if [ "$FORCE" -eq 0 -a "$SOURCEONLY" -eq 0 -a "$NOBUILD" -eq 0 ]; then - if [ "$allpkgbuilt" -eq 1 ]; then - if [ "$INSTALL" -eq 1 ]; then + if ! (( FORCE || SOURCEONLY || NOBUILD )); then + if (( allpkgbuilt )); then + if (( INSTALL )); then warning "$(gettext "The package group has already been built, installing existing packages...")" install_package exit $? @@ -1741,7 +1740,7 @@ else exit 1 fi fi - if [ "$somepkgbuilt" -eq 1 ]; then + if (( somepkgbuilt )); then error "$(gettext "Part of the package group has already been built. (use -f to overwrite)")" exit 1 fi @@ -1750,10 +1749,10 @@ else fi # Run the bare minimum in fakeroot -if [ "$INFAKEROOT" -eq 1 ]; then - if [ "$SPLITPKG" -eq 0 ]; then - if [ "$PKGFUNC" -eq 0 ]; then - if [ "$REPKG" -eq 0 ]; then +if (( INFAKEROOT )); then + if (( ! SPLITPKG )); then + if (( ! PKGFUNC )); then + if (( ! REPKG )); then run_build tidy_install fi @@ -1782,9 +1781,9 @@ fi msg "$(gettext "Making package: %s")" "$pkgbase $pkgver-$pkgrel ($(date))" # if we are creating a source-only package, go no further -if [ "$SOURCEONLY" -ne 0 ]; then - if [ -f "$PKGDEST/${pkgbase}-${pkgver}-${pkgrel}${SRCEXT}" \ - -a "$FORCE" -eq 0 ]; then +if (( SOURCEONLY )); then + if [[ -f $PKGDEST/${pkgbase}-${pkgver}-${pkgrel}${SRCEXT} ]] \ + && (( ! FORCE )); then error "$(gettext "A package has already been built. (use -f to overwrite)")" exit 1 fi @@ -1794,9 +1793,9 @@ if [ "$SOURCEONLY" -ne 0 ]; then fi # fix flyspray bug #5973 -if [ "$NODEPS" -eq 1 -o "$NOBUILD" -eq 1 -o "$REPKG" -eq 1 ]; then +if (( NODEPS || NOBUILD || REPKG )); then # no warning message needed for nobuild, repkg - if [ "$NODEPS" -eq 1 ]; then + if (( NODEPS )); then warning "$(gettext "Skipping dependency checks.")" fi elif [ $(type -p pacman) ]; then @@ -1809,7 +1808,7 @@ elif [ $(type -p pacman) ]; then msg "$(gettext "Checking Buildtime Dependencies...")" resolve_deps ${makedepends[@]} || deperr=1 - if [ $deperr -eq 1 ]; then + if (( deperr )); then error "$(gettext "Could not resolve all dependencies.")" exit 1 fi @@ -1824,19 +1823,19 @@ umask 0022 mkdir -p "$srcdir" cd "$srcdir" -if [ "$NOEXTRACT" -eq 1 ]; then +if (( NOEXTRACT )); then warning "$(gettext "Skipping source retrieval -- using existing src/ tree")" warning "$(gettext "Skipping source integrity checks -- using existing src/ tree")" warning "$(gettext "Skipping source extraction -- using existing src/ tree")" - if [ "$NOEXTRACT" -eq 1 -a -z "$(ls "$srcdir" 2>/dev/null)" ]; then + if (( NOEXTRACT )) && [[ -z $(ls "$srcdir" 2>/dev/null) ]]; then error "$(gettext "The source directory is empty, there is nothing to build!")" plain "$(gettext "Aborting...")" exit 1 fi -elif [ "$REPKG" -eq 1 ]; then - if [ "$PKGFUNC" -eq 0 -a "$SPLITPKG" -eq 0 \ - -a \( ! -d "$pkgdir" -o -z "$(ls "$pkgdir" 2>/dev/null)" \) ]; then +elif (( REPKG )); then + if (( ! PKGFUNC && ! SPLITPKG )) \ + && [[ ! -d $pkgdir || -z $(ls "$pkgdir" 2>/dev/null) ]]; then error "$(gettext "The package directory is empty, there is nothing to repackage!")" plain "$(gettext "Aborting...")" exit 1 @@ -1847,13 +1846,12 @@ else extract_sources fi -if [ "$NOBUILD" -eq 1 ]; then +if (( NOBUILD )); then msg "$(gettext "Sources are ready.")" exit 0 #E_OK else # check for existing pkg directory; don't remove if we are repackaging - if [ -d "$pkgdir" \ - -a \( "$REPKG" -eq 0 -o "$PKGFUNC" -eq 1 -o "$SPLITPKG" -eq 1 \) ]; then + if [[ -d $pkgdir ]] && (( ! REPKG || PKGFUNC || SPLITPKG )); then msg "$(gettext "Removing existing pkg/ directory...")" rm -rf "$pkgdir" fi @@ -1861,16 +1859,16 @@ else cd "$startdir" # if we are root or if fakeroot is not enabled, then we don't use it - if [ "$(check_buildenv fakeroot)" != "y" -o $EUID -eq 0 ]; then - if [ "$REPKG" -eq 0 ]; then + if [[ $(check_buildenv fakeroot) != "y" ]] || (( EUID == 0 )); then + if (( ! REPKG )); then devel_update run_build fi - if [ "$SPLITPKG" -eq 0 ]; then - if [ "$PKGFUNC" -eq 1 ]; then + if (( ! SPLITPKG )); then + if (( PKGFUNC )); then run_package tidy_install - elif [ "$REPKG" -eq 0 ]; then + elif (( ! REPKG )); then tidy_install fi create_package @@ -1887,7 +1885,7 @@ else done fi else - if [ "$REPKG" -eq 0 -a \( "$PKGFUNC" -eq 1 -o "$SPLITPKG" -eq 1 \) ]; then + if (( ! REPKG && ( PKGFUNC || SPLITPKG ) )); then devel_update run_build cd "$startdir" @@ -1895,7 +1893,7 @@ else msg "$(gettext "Entering fakeroot environment...")" - if [ -n "$newpkgver" ]; then + if [[ -n $newpkgver ]]; then fakeroot -- $0 --forcever $newpkgver -F "${ARGLIST[@]}" || exit $? else fakeroot -- $0 -F "${ARGLIST[@]}" || exit $? -- 1.6.5.2
Isaac Good wrote:
From e3e5bb93cb2b33315a8d6eff8956aa5b6c74d28d Mon Sep 17 00:00:00 2001 From: Isaac Good <pacman@isaac.otherinbox.com> Date: Thu, 12 Nov 2009 15:09:05 -0500 Subject: [PATCH 2/2] Changing [ to [[ and (( - Part Two - Amended
Second half of makepkg This replaces any prior patches of mine Includes stuff like -o to || and -a to && etc if [ $(type .. were preserved due to a bash bug with [[ and set -e and ERR trap
Signed-off-by: Isaac Good <pacman@isaac.otherinbox.com>
<snip> I have pushed these two patches and Cedric's one for the other scripts to the "bashtest" branch in my repo [1]. I am going through these now and will get back with any queries. Allan [1] http://projects.archlinux.org/users/allan/pacman.git/log/?h=bashtest
I have looked at the two patches and the good news is that I can spot nothing wrong! I just have a few comments about style that I would like to discuss. I would like to get some more consistency in quoting. e.g. here are some varied tests: - if [ "$ret" != '?' ]; then + if [[ $ret != '?' ]]; then + if [[ $ret != "?" ]]; then + if [[ $ret = y ]]; then - if [ "$cmd" = "bsdtar" ]; then + if [[ $cmd = bsdtar ]]; then - if [ "$(check_option makeflags)" = "n" ]; then + if [[ $(check_option makeflags) = "n" ]]; then - if [ "$arch" != 'any' ]; then + if [[ $arch != 'any' ]]; then - if [ ${1:0:2} = '--' ]; then + if [[ ${1:0:2} = '--' ]]; then - if [ -d ./src/$_hgrepo ] ; then + if [[ -d ./src/$_hgrepo ]] ; then -if [ -r ~/.makepkg.conf ]; then +if [[ -r ~/.makepkg.conf ]]; then My suggestion is that any thing with text (i.e. not a pure variable) is quoted. I know this is excessive in some cases (e.g. the last case) but the only exception I would be happy with is tests that are pure paths with only added "/" (e.g. $startdir/$file). Even then, maybe quotes would be nicer... I am happy to be debated on this. There should be quotes kept in the gettext calls in this test: - if [ "$answer" = "$(gettext "YES")" -o "$answer" = "$(gettext "Y")" ]; then + if [[ $answer = $(gettext YES) || $answer = $(gettext Y) ]]; then Why is EUID tested against 0 explicitly when all other tests for zero just use ! EUID? e.g. - if [ $EUID -eq 0 -a "$ASROOT" -eq 0 ]; then + if (( EUID == 0 && ! ASROOT )); then In fact, I quite like that things are explicitly tested in this case. I wonder if the tests of return values should explicitly test for "== 0" or "!= 0". e.g. these test have become less clear to me when I read the code. - if [ $ret -gt 0 ]; then + if (( ret )); then - elif [ $ret -ne 0 ]; then + elif (( ret )); then Note that these explicitly test for "== 0" or "> 0" and I think that is much clearer: - [ $# -gt 0 ] || return + (( $# > 0 )) || return - [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED Allan
On Sun, Nov 15, 2009 at 02:40:47PM +0000, Allan McRae wrote:
I have looked at the two patches and the good news is that I can spot nothing wrong! I just have a few comments about style that I would like to discuss.
I would like to get some more consistency in quoting. e.g. here are some varied tests:
- if [ "$ret" != '?' ]; then + if [[ $ret != '?' ]]; then
+ if [[ $ret != "?" ]]; then + if [[ $ret = y ]]; then
- if [ "$cmd" = "bsdtar" ]; then + if [[ $cmd = bsdtar ]]; then
- if [ "$(check_option makeflags)" = "n" ]; then + if [[ $(check_option makeflags) = "n" ]]; then
- if [ "$arch" != 'any' ]; then + if [[ $arch != 'any' ]]; then
- if [ ${1:0:2} = '--' ]; then + if [[ ${1:0:2} = '--' ]]; then
- if [ -d ./src/$_hgrepo ] ; then + if [[ -d ./src/$_hgrepo ]] ; then
-if [ -r ~/.makepkg.conf ]; then +if [[ -r ~/.makepkg.conf ]]; then
My suggestion is that any thing with text (i.e. not a pure variable) is quoted. I know this is excessive in some cases (e.g. the last case) but the only exception I would be happy with is tests that are pure paths with only added "/" (e.g. $startdir/$file). Even then, maybe quotes would be nicer... I am happy to be debated on this.
That "n" and 'any' slipped my radar. I tried removing quotes for simple strings. ? looks like a bash pattern so I left it quotes. -- is also "different".
There should be quotes kept in the gettext calls in this test: - if [ "$answer" = "$(gettext "YES")" -o "$answer" = "$(gettext "Y")" ]; then + if [[ $answer = $(gettext YES) || $answer = $(gettext Y) ]]; then
Perhaps? Around the arguments passed to gettext or around the $( )?
Why is EUID tested against 0 explicitly when all other tests for zero just use ! EUID? e.g.
- if [ $EUID -eq 0 -a "$ASROOT" -eq 0 ]; then + if (( EUID == 0 && ! ASROOT )); then
In fact, I quite like that things are explicitly tested in this case.
I wonder if the tests of return values should explicitly test for "== 0" or "!= 0". e.g. these test have become less clear to me when I read the code.
- if [ $ret -gt 0 ]; then + if (( ret )); then
- elif [ $ret -ne 0 ]; then + elif (( ret )); then
Note that these explicitly test for "== 0" or "> 0" and I think that is much clearer:
- [ $# -gt 0 ] || return + (( $# > 0 )) || return
- [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED
As mentioned earlier, most the variables tested are being used as booleans. Either TRUE or FALSE. Hence: if (( FLAG )) then the flag is set. Similar to C/Java/Perl/etc The EUID and $# are not flags. They are integer values. So I left the test explicit. - Isaac
On Sun, Nov 15, 2009 at 8:40 AM, Allan McRae <allan@archlinux.org> wrote:
I have looked at the two patches and the good news is that I can spot nothing wrong! I just have a few comments about style that I would like to discuss.
I would like to get some more consistency in quoting. e.g. here are some varied tests:
- if [ "$ret" != '?' ]; then + if [[ $ret != '?' ]]; then
+ if [[ $ret != "?" ]]; then + if [[ $ret = y ]]; then
- if [ "$cmd" = "bsdtar" ]; then + if [[ $cmd = bsdtar ]]; then
- if [ "$(check_option makeflags)" = "n" ]; then + if [[ $(check_option makeflags) = "n" ]]; then
- if [ "$arch" != 'any' ]; then + if [[ $arch != 'any' ]]; then
- if [ ${1:0:2} = '--' ]; then + if [[ ${1:0:2} = '--' ]]; then
- if [ -d ./src/$_hgrepo ] ; then + if [[ -d ./src/$_hgrepo ]] ; then
-if [ -r ~/.makepkg.conf ]; then +if [[ -r ~/.makepkg.conf ]]; then
My suggestion is that any thing with text (i.e. not a pure variable) is quoted. I know this is excessive in some cases (e.g. the last case) but the only exception I would be happy with is tests that are pure paths with only added "/" (e.g. $startdir/$file). Even then, maybe quotes would be nicer... I am happy to be debated on this.
There should be quotes kept in the gettext calls in this test: - if [ "$answer" = "$(gettext "YES")" -o "$answer" = "$(gettext "Y")" ]; then + if [[ $answer = $(gettext YES) || $answer = $(gettext Y) ]]; then
Why is EUID tested against 0 explicitly when all other tests for zero just use ! EUID? e.g.
- if [ $EUID -eq 0 -a "$ASROOT" -eq 0 ]; then + if (( EUID == 0 && ! ASROOT )); then
In fact, I quite like that things are explicitly tested in this case.
I wonder if the tests of return values should explicitly test for "== 0" or "!= 0". e.g. these test have become less clear to me when I read the code.
- if [ $ret -gt 0 ]; then + if (( ret )); then
- elif [ $ret -ne 0 ]; then + elif (( ret )); then
Note that these explicitly test for "== 0" or "> 0" and I think that is much clearer:
- [ $# -gt 0 ] || return + (( $# > 0 )) || return
- [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED
As a side note, in the pacman C code we try to stick to a convention where if testing for 0 makes sense, do it explicitly. This comes up often in the case of strcmp() == 0, which means "these two strings match". e.g. if(strcmp(pkgname, name) == 0) { instead of if(!strcmp(pkgname, name)) { I like the logic here of doing explicit declarations of 0 in the EUID case. For a true boolean in the other cases, I'm fine with dropping the zero check. -Dan
Allan McRae wrote:
I have looked at the two patches and the good news is that I can spot nothing wrong! I just have a few comments about style that I would like to discuss.
[...]
My suggestion is that any thing with text (i.e. not a pure variable) is quoted. I know this is excessive in some cases (e.g. the last case) but the only exception I would be happy with is tests that are pure paths with only added "/" (e.g. $startdir/$file). Even then, maybe quotes would be nicer... I am happy to be debated on this.
To quote myself [1]:
In my opinion, it is often not obvious whether they are required or not which is why I tended to use more quotes than actually were needed. I am fairly familiar with quoting by now and can use both "quoting styles", but I think it would probably be a good idea to decide for one. Using just as much quotes as required would be a little bit shorter, but using quotes `where possible` may be a little bit more foolproof.
Meaning that I am fine with quoting strings generally. [1] http://mailman.archlinux.org/pipermail/pacman-dev/2009-November/010115.html
I wonder if the tests of return values should explicitly test for "== 0" or "!= 0". e.g. these test have become less clear to me when I read the code.
- if [ $ret -gt 0 ]; then + if (( ret )); then
- elif [ $ret -ne 0 ]; then + elif (( ret )); then
Yep, sounds reasonable, especially since we already explicitly test all (or at least many) other non-boolean variables.
participants (5)
-
Allan McRae
-
Cedric Staniewski
-
Dan McGee
-
Isaac Good
-
Jeff