[pacman-dev] [PATCH] [makepkg] use double brackets
Signed-off-by: Andres P <aepd87@gmail.com> --- scripts/makepkg.sh.in | 32 ++++++++++++++++---------------- 1 files changed, 16 insertions(+), 16 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 1707245..be076a7 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -345,7 +345,7 @@ download_file() { run_pacman() { local ret=0 if (( ! ASROOT )) && [[ $1 != "-T" && $1 != "-Qq" ]]; then - if [ "$(type -p sudo)" ] && sudo -l $PACMAN &>/dev/null; then + if [[ $(type -p sudo) ]] && sudo -l $PACMAN &>/dev/null; then sudo $PACMAN $PACMAN_OPTS "$@" || ret=$? else su -c "$PACMAN $PACMAN_OPTS $*" || ret=$? @@ -519,7 +519,7 @@ generate_checksums() { msg "$(gettext "Generating checksums for source files...")" plain "" - if [ ! $(type -p openssl) ]; then + if [[ ! $(type -p openssl) ]]; then error "$(gettext "Cannot find openssl.")" exit 1 # $E_MISSING_PROGRAM fi @@ -580,7 +580,7 @@ generate_checksums() { check_checksums() { (( ! ${#source[@]} )) && return 0 - if [ ! $(type -p openssl) ]; then + if [[ ! $(type -p openssl) ]]; then error "$(gettext "Cannot find openssl.")" exit 1 # $E_MISSING_PROGRAM fi @@ -1296,7 +1296,7 @@ check_sanity() { if (( ${#pkgname[@]} > 1 )); then for pkg in ${pkgname[@]}; do - if [ "$(type -t package_${pkg})" != "function" ]; then + if [[ $(type -t package_${pkg}) != "function" ]]; then error "$(gettext "missing package function for split package '%s'")" "$pkg" return 1 fi @@ -1333,27 +1333,27 @@ devel_check() { # Also do a brief check to make sure we have the VCS tool available. oldpkgver=$pkgver if [[ -n ${_darcstrunk} && -n ${_darcsmod} ]] ; then - [ $(type -p darcs) ] || return 0 + [[ $(type -p darcs) ]] || return 0 msg "$(gettext "Determining latest darcs revision...")" newpkgver=$(date +%Y%m%d) elif [[ -n ${_cvsroot} && -n ${_cvsmod} ]] ; then - [ $(type -p cvs) ] || return 0 + [[ $(type -p cvs) ]] || return 0 msg "$(gettext "Determining latest cvs revision...")" newpkgver=$(date +%Y%m%d) elif [[ -n ${_gitroot} && -n ${_gitname} ]] ; then - [ $(type -p git) ] || return 0 + [[ $(type -p git) ]] || return 0 msg "$(gettext "Determining latest git revision...")" newpkgver=$(date +%Y%m%d) elif [[ -n ${_svntrunk} && -n ${_svnmod} ]] ; then - [ $(type -p svn) ] || return 0 + [[ $(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} && -n ${_bzrmod} ]] ; then - [ $(type -p bzr) ] || return 0 + [[ $(type -p bzr) ]] || return 0 msg "$(gettext "Determining latest bzr revision...")" newpkgver=$(bzr revno ${_bzrtrunk}) elif [[ -n ${_hgroot} && -n ${_hgrepo} ]] ; then - [ $(type -p hg) ] || return 0 + [[ $(type -p hg) ]] || return 0 msg "$(gettext "Determining latest hg revision...")" if [[ -d ./src/$_hgrepo ]] ; then cd ./src/$_hgrepo @@ -1555,7 +1555,7 @@ There is NO WARRANTY, to the extent permitted by law.\n")" # PROGRAM START # determine whether we have gettext; make it a no-op if we do not -if [ ! $(type -t gettext) ]; then +if [[ ! $(type -t gettext) ]]; then gettext() { echo "$@" } @@ -1729,7 +1729,7 @@ if (( ! INFAKEROOT )); then plain "$(gettext "Please rerun makepkg without the --asroot flag.")" exit 1 # $E_USER_ABORT elif [[ $(check_buildenv fakeroot) = "y" ]] && (( EUID > 0 )); then - if [ ! $(type -p fakeroot) ]; 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 @@ -1749,7 +1749,7 @@ fi # check for sudo if we will need it during makepkg execution if (( ! ( ASROOT || INFAKEROOT ) && ( DEP_BIN || RMDEPS || INSTALL ) )); then - if [ ! "$(type -p sudo)" ]; then + if [[ ! $(type -p sudo) ]]; then warning "$(gettext "Sudo can not be found. Will use su to acquire root privileges.")" fi fi @@ -1810,9 +1810,9 @@ fi if [[ $(! type -t build) = "function" ]]; then BUILDFUNC=1 fi -if [ "$(type -t package)" = "function" ]; then +if [[ $(type -t package) = "function" ]]; then PKGFUNC=1 -elif [ $SPLITPKG -eq 0 -a "$(type -t package_${pkgname})" = "function" ]; then +elif [[ $SPLITPKG -eq 0 && $(type -t package_${pkgname}) = "function" ]]; then SPLITPKG=1 fi @@ -1921,7 +1921,7 @@ if (( NODEPS || ( (NOBUILD || REPKG) && !DEP_BIN ) )); then if (( NODEPS || ( REPKG && PKGFUNC ) )); then warning "$(gettext "Skipping dependency checks.")" fi -elif [ $(type -p "${PACMAN%% *}") ]; then +elif [[ $(type -p "${PACMAN%% *}") ]]; then if (( RMDEPS )); then original_pkglist=($(run_pacman -Qq | sort)) # required by remove_dep fi -- 1.7.1
Looked like slackware's makepkg Signed-off-by: Andres P <aepd87@gmail.com> --- Maybe it should be a function: sed_bscript() { sed -n "s/^\([[:space:]]*\)$1=/\1/p" "$BUILDSCRIPT"; } Then there's only one regex to maintain: local install_files=($(sed_bscript install)) local changelog_files=($(sed_bscript changelog)) ... scripts/makepkg.sh.in | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 1707245..5021fe9 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -318,12 +318,12 @@ download_file() { local dlfile="${url##*/}" # replace %o by the temporary dlfile if it exists - if echo "$dlcmd" | grep -q "%o" ; then + if [[ $dlcmd = *%o* ]]; then dlcmd=${dlcmd//\%o/\"$file.part\"} dlfile="$file.part" fi # add the URL, either in place of %u or at the end - if echo "$dlcmd" | grep -q "%u" ; then + if [[ $dlcmd = *%u* ]]; then dlcmd=${dlcmd//\%u/\"$url\"} else dlcmd="$dlcmd \"$url\"" @@ -1107,7 +1107,7 @@ create_srcpackage() { fi done - local install_files=( $(grep "^[[:space:]]*install=" "$BUILDSCRIPT" | sed "s/install=//") ) + local install_files=( $(sed -n "s/^\([[:space:]]*\)install=/\1/p" "$BUILDSCRIPT") ) if [[ -n $install_files ]]; then local file for file in ${install_files[@]}; do @@ -1120,7 +1120,7 @@ create_srcpackage() { done fi - local changelog_files=( $(grep "^[[:space:]]*changelog=" "$BUILDSCRIPT" | sed "s/changelog=//") ) + local changelog_files=( $(sed -n "s/^\([[:space:]]*\)changelog=/\1/p" "$BUILDSCRIPT") ) if [[ -n $changelog_files ]]; then local file for file in ${changelog_files[@]}; do @@ -1250,7 +1250,7 @@ check_sanity() { fi done - local install_files=( $(grep "^[[:space:]]*install=" "$BUILDSCRIPT" | sed "s/install=//") ) + local install_files=( $(sed -n "s/^\([[:space:]]*\)install=/\1/p" "$BUILDSCRIPT") ) if [[ -n $install_files ]]; then local file for file in ${install_files[@]}; do @@ -1263,7 +1263,7 @@ check_sanity() { done fi - local changelog_files=( $(grep "^[[:space:]]*changelog=" "$BUILDSCRIPT" | sed "s/changelog=//") ) + local changelog_files=( $(sed -n "s/^\([[:space:]]*\)changelog=/\1/p" "$BUILDSCRIPT") ) if [[ -n $changelog_files ]]; then for file in ${changelog_files[@]}; do # evaluate any bash variables used @@ -1572,7 +1572,7 @@ OPT_LONG="$OPT_LONG,source,syncdeps,version,config:" # Pacman Options OPT_LONG="$OPT_LONG,noconfirm,noprogressbar" OPT_TEMP="$(parse_options $OPT_SHORT $OPT_LONG "$@" || echo 'PARSE_OPTIONS FAILED')" -if echo "$OPT_TEMP" | grep -q 'PARSE_OPTIONS FAILED'; then +if [[ $OPT_TEMP = *'PARSE_OPTIONS FAILED'* ]]; then # This is a small hack to stop the script bailing with 'set -e' echo; usage; exit 1 # E_INVALID_OPTION; fi -- 1.7.1
On 22/05/10 15:06, Andres P wrote:
Looked like slackware's makepkg
Signed-off-by: Andres P<aepd87@gmail.com>
Pushed to my working branch. Although removed the commit message as it had absolutely no relevance.
---
Maybe it should be a function: sed_bscript() { sed -n "s/^\([[:space:]]*\)$1=/\1/p" "$BUILDSCRIPT"; } Then there's only one regex to maintain: local install_files=($(sed_bscript install)) local changelog_files=($(sed_bscript changelog))
I thought about this when committing the original grep regex but came to the conclusion that it is only four locations and are quite unlikely to change often, so I think it will be fine to leave it as is. Allan
On 22/05/10 15:06, Andres P wrote:
@@ -1810,9 +1810,9 @@ fi if [[ $(! type -t build) = "function" ]]; then BUILDFUNC=1 fi -if [ "$(type -t package)" = "function" ]; then +if [[ $(type -t package) = "function" ]]; then PKGFUNC=1 -elif [ $SPLITPKG -eq 0 -a "$(type -t package_${pkgname})" = "function" ]; then +elif [[ $SPLITPKG -eq 0&& $(type -t package_${pkgname}) = "function" ]]; then SPLITPKG=1 fi
Note that line at the top there has an "!" in the type statement. That is needed for bash-4.0 compatibility. This comment is just above it: # test for available PKGBUILD functions # The exclamation mark is required here to avoid triggering the ERR trap when # a tested function does not exist. I'm not sure we can break bash-4.0 compatibility yet... Allan
On 22/05/10 15:26, Allan McRae wrote:
On 22/05/10 15:06, Andres P wrote:
@@ -1810,9 +1810,9 @@ fi if [[ $(! type -t build) = "function" ]]; then BUILDFUNC=1 fi -if [ "$(type -t package)" = "function" ]; then +if [[ $(type -t package) = "function" ]]; then PKGFUNC=1 -elif [ $SPLITPKG -eq 0 -a "$(type -t package_${pkgname})" = "function" ]; then +elif [[ $SPLITPKG -eq 0&& $(type -t package_${pkgname}) = "function" ]]; then SPLITPKG=1 fi
Note that line at the top there has an "!" in the type statement. That is needed for bash-4.0 compatibility. This comment is just above it:
# test for available PKGBUILD functions # The exclamation mark is required here to avoid triggering the ERR trap when # a tested function does not exist.
I'm not sure we can break bash-4.0 compatibility yet...
In fact, thinking about this more. Bash-4.1 was released on 2010-01-03 so it will have been out for ~6 months before the next pacman release. Anyone upgrading their package manager from 3.3 -> 3.4 will upgrade bash from 4.0 -> 4.1 so this is probably a non-issue. So I will accept this patch when the comment above and "stray" ! is removed with it. Allan
On Sat, May 22, 2010 at 1:48 AM, Allan McRae <allan@archlinux.org> wrote:
On 22/05/10 15:26, Allan McRae wrote:
On 22/05/10 15:06, Andres P wrote:
@@ -1810,9 +1810,9 @@ fi if [[ $(! type -t build) = "function" ]]; then BUILDFUNC=1 fi -if [ "$(type -t package)" = "function" ]; then +if [[ $(type -t package) = "function" ]]; then PKGFUNC=1 -elif [ $SPLITPKG -eq 0 -a "$(type -t package_${pkgname})" = "function" ]; then +elif [[ $SPLITPKG -eq 0&& $(type -t package_${pkgname}) = "function" ]]; then SPLITPKG=1 fi
Note that line at the top there has an "!" in the type statement. That is needed for bash-4.0 compatibility. This comment is just above it:
# test for available PKGBUILD functions # The exclamation mark is required here to avoid triggering the ERR trap when # a tested function does not exist.
I'm not sure we can break bash-4.0 compatibility yet...
In fact, thinking about this more. Bash-4.1 was released on 2010-01-03 so it will have been out for ~6 months before the next pacman release. Anyone upgrading their package manager from 3.3 -> 3.4 will upgrade bash from 4.0 -> 4.1 so this is probably a non-issue.
So I will accept this patch when the comment above and "stray" ! is removed with it.
You are assuming Arch users only here; has Cygwin and OS X moved from bash 3.X to 4.X yet? I'd rather not break this just for the sake of getting this patch in. -Dan
On Sat, May 22, 2010 at 5:14 PM, Dan McGee <dpmcgee@gmail.com> wrote:
On Sat, May 22, 2010 at 1:48 AM, Allan McRae <allan@archlinux.org> wrote:
In fact, thinking about this more. Bash-4.1 was released on 2010-01-03 so it will have been out for ~6 months before the next pacman release. Anyone upgrading their package manager from 3.3 -> 3.4 will upgrade bash from 4.0 -> 4.1 so this is probably a non-issue.
So I will accept this patch when the comment above and "stray" ! is removed with it.
You are assuming Arch users only here; has Cygwin and OS X moved from bash 3.X to 4.X yet? I'd rather not break this just for the sake of getting this patch in.
-Dan
Damn I knew there was some mails I meant to reply and forgot about. If it's that easy to remain compatibility with bash 3, better keep it that way. IMO it's fine to drop support for bash 3 as soon as it becomes a burden and we have a good reason for requiring bash 4. But that does not seem to be the case for now.
On Sat, May 22, 2010 at 10:53 AM, Xavier Chantry <chantry.xavier@gmail.com> wrote:
On Sat, May 22, 2010 at 5:14 PM, Dan McGee <dpmcgee@gmail.com> wrote:
On Sat, May 22, 2010 at 1:48 AM, Allan McRae <allan@archlinux.org> wrote:
In fact, thinking about this more. Bash-4.1 was released on 2010-01-03 so it will have been out for ~6 months before the next pacman release. Anyone upgrading their package manager from 3.3 -> 3.4 will upgrade bash from 4.0 -> 4.1 so this is probably a non-issue.
So I will accept this patch when the comment above and "stray" ! is removed with it.
You are assuming Arch users only here; has Cygwin and OS X moved from bash 3.X to 4.X yet? I'd rather not break this just for the sake of getting this patch in.
-Dan
Damn I knew there was some mails I meant to reply and forgot about.
If it's that easy to remain compatibility with bash 3, better keep it that way. IMO it's fine to drop support for bash 3 as soon as it becomes a burden and we have a good reason for requiring bash 4. But that does not seem to be the case for now.
I'm kinda lost here ;) How does it break compat with bash 4? -- Andres P
On 23/05/10 01:23, Xavier Chantry wrote:
On Sat, May 22, 2010 at 5:14 PM, Dan McGee<dpmcgee@gmail.com> wrote:
On Sat, May 22, 2010 at 1:48 AM, Allan McRae<allan@archlinux.org> wrote:
In fact, thinking about this more. Bash-4.1 was released on 2010-01-03 so it will have been out for ~6 months before the next pacman release. Anyone upgrading their package manager from 3.3 -> 3.4 will upgrade bash from 4.0 -> 4.1 so this is probably a non-issue.
So I will accept this patch when the comment above and "stray" ! is removed with it.
You are assuming Arch users only here; has Cygwin and OS X moved from bash 3.X to 4.X yet? I'd rather not break this just for the sake of getting this patch in.
-Dan
Damn I knew there was some mails I meant to reply and forgot about.
If it's that easy to remain compatibility with bash 3, better keep it that way. IMO it's fine to drop support for bash 3 as soon as it becomes a burden and we have a good reason for requiring bash 4. But that does not seem to be the case for now.
This does not break compatibility with bash-3.x. Just bash-4.0 which had a bug in it in the way it handles the type tests. In bash-4.0 _only_: if [[ $(type -t build) == "function" ]] sets of the error trap if the build function does not exist. In all other version of bash, that works fine. I am fine with dropping compatibility with bash-4.0 because any distribution (am I am including MacOSX and cygwin under that banner) that upgrades to bash-4 now will not be using bash-4.0. Allan
On Sun, May 23, 2010 at 1:04 AM, Allan McRae <allan@archlinux.org> wrote:
This does not break compatibility with bash-3.x. Just bash-4.0 which had a bug in it in the way it handles the type tests.
In bash-4.0 _only_: if [[ $(type -t build) == "function" ]] sets of the error trap if the build function does not exist. In all other version of bash, that works fine.
I am fine with dropping compatibility with bash-4.0 because any distribution (am I am including MacOSX and cygwin under that banner) that upgrades to bash-4 now will not be using bash-4.0.
Ah ok, I read that mail too quickly and misunderstood, it's much clearer now :) And completely fine then.
On Sat, May 22, 2010 at 2:18 AM, Allan McRae <allan@archlinux.org> wrote:
In fact, thinking about this more. Bash-4.1 was released on 2010-01-03 so it will have been out for ~6 months before the next pacman release. Anyone upgrading their package manager from 3.3 -> 3.4 will upgrade bash from 4.0 -> 4.1 so this is probably a non-issue.
So I will accept this patch when the comment above and "stray" ! is removed with it.
Allan
I was about to do that, but first I think I should talk about another change. `type -p foo` has a return val, so consider this: $ time for i in {1..1000}; do [[ $(type -p sh) ]]; done real 0m1.564s user 0m0.160s sys 0m0.337s $ time for i in {1..1000}; do type -p sh &>/dev/null; done real 0m0.166s user 0m0.060s sys 0m0.103s Even though there is a negligible perfomance difference in makepkg, this is the correct way to check for $?. If the redir to /dev/null becomes to much of a burden to type, it could become a function. Not to mention that the whole series of checking for vcs in PATH, which is where most of `type p` gets used, could be changed in to a more platable chunk. Would there be interest in making this change? I really think that throwing away the return val and making a string check is poor practice. -- Andres P
On Tue, May 25, 2010 at 10:06 PM, Andres P <aepd87@gmail.com> wrote:
On Sat, May 22, 2010 at 2:18 AM, Allan McRae <allan@archlinux.org> wrote:
In fact, thinking about this more. Bash-4.1 was released on 2010-01-03 so it will have been out for ~6 months before the next pacman release. Anyone upgrading their package manager from 3.3 -> 3.4 will upgrade bash from 4.0 -> 4.1 so this is probably a non-issue.
So I will accept this patch when the comment above and "stray" ! is removed with it.
Allan
I was about to do that, but first I think I should talk about another change.
`type -p foo` has a return val, so consider this: $ time for i in {1..1000}; do [[ $(type -p sh) ]]; done real 0m1.564s user 0m0.160s sys 0m0.337s
$ time for i in {1..1000}; do type -p sh &>/dev/null; done real 0m0.166s user 0m0.060s sys 0m0.103s
Even though there is a negligible perfomance difference in makepkg, this is the correct way to check for $?.
I don't understand this sentence. negligible = 10x ? And if your "correct" way is 10x faster, why do you say "even though" ? This makes it sound like we need to accept a compromise (like worse perf) for a correct behavior. But from my understanding of your mail, you are saying that using type -p directly is both more correct and faster.
If the redir to /dev/null becomes to much of a burden to type, it could become a function. Not to mention that the whole series of checking for vcs in PATH, which is where most of `type p` gets used, could be changed in to a more platable chunk.
Would there be interest in making this change?
I really think that throwing away the return val and making a string check is poor practice.
So the only downside of using type -p sh &>/dev/null directly without [/[[ and a subshell is that we need to type &>/dev/null ? It does not seem like a big deal to me. And yeah if it's used a lot, like more than 5 times, just make a function.
On Tue, May 25, 2010 at 5:16 PM, Xavier Chantry <chantry.xavier@gmail.com> wrote:
On Tue, May 25, 2010 at 10:06 PM, Andres P <aepd87@gmail.com> wrote:
I was about to do that, but first I think I should talk about another change.
`type -p foo` has a return val, so consider this: $ time for i in {1..1000}; do [[ $(type -p sh) ]]; done real 0m1.564s user 0m0.160s sys 0m0.337s
$ time for i in {1..1000}; do type -p sh &>/dev/null; done real 0m0.166s user 0m0.060s sys 0m0.103s
Even though there is a negligible perfomance difference in makepkg, this is the correct way to check for $?.
I don't understand this sentence. negligible = 10x ? And if your "correct" way is 10x faster, why do you say "even though" ? This makes it sound like we need to accept a compromise (like worse perf) for a correct behavior. But from my understanding of your mail, you are saying that using type -p directly is both more correct and faster.
I'm saying negligible *in* makepkg. I'm making it very clear that the justification for a change is not performance. Obviously, the same scale on that timediff could be applied to makepkg, but even then, the gains from implementing my suggestion are unperceivable.
If the redir to /dev/null becomes to much of a burden to type, it could become a function. Not to mention that the whole series of checking for vcs in PATH, which is where most of `type p` gets used, could be changed in to a more platable chunk.
Would there be interest in making this change?
I really think that throwing away the return val and making a string check is poor practice.
So the only downside of using type -p sh &>/dev/null directly without [/[[ and a subshell is that we need to type &>/dev/null ? It does not seem like a big deal to me. And yeah if it's used a lot, like more than 5 times, just make a function.
Just to be absolutely clear, I prefer &>/dev/null over the current way of handling this. This is why I proposed this. ;) -- Andres P
On 26/05/10 08:22, Andres P wrote:
On Tue, May 25, 2010 at 5:16 PM, Xavier Chantry <chantry.xavier@gmail.com> wrote:
On Tue, May 25, 2010 at 10:06 PM, Andres P<aepd87@gmail.com> wrote:
I was about to do that, but first I think I should talk about another change.
`type -p foo` has a return val, so consider this: $ time for i in {1..1000}; do [[ $(type -p sh) ]]; done real 0m1.564s user 0m0.160s sys 0m0.337s
$ time for i in {1..1000}; do type -p sh&>/dev/null; done real 0m0.166s user 0m0.060s sys 0m0.103s
Even though there is a negligible perfomance difference in makepkg, this is the correct way to check for $?.
I don't understand this sentence. negligible = 10x ? And if your "correct" way is 10x faster, why do you say "even though" ? This makes it sound like we need to accept a compromise (like worse perf) for a correct behavior. But from my understanding of your mail, you are saying that using type -p directly is both more correct and faster.
I'm saying negligible *in* makepkg. I'm making it very clear that the justification for a change is not performance. Obviously, the same scale on that timediff could be applied to makepkg, but even then, the gains from implementing my suggestion are unperceivable.
If the redir to /dev/null becomes to much of a burden to type, it could become a function. Not to mention that the whole series of checking for vcs in PATH, which is where most of `type p` gets used, could be changed in to a more platable chunk.
Would there be interest in making this change?
I really think that throwing away the return val and making a string check is poor practice.
So the only downside of using type -p sh&>/dev/null directly without [/[[ and a subshell is that we need to type&>/dev/null ? It does not seem like a big deal to me. And yeah if it's used a lot, like more than 5 times, just make a function.
Just to be absolutely clear, I prefer&>/dev/null over the current way of handling this.
This is why I proposed this. ;)
I agree with the change. Feel free to change all the "[[ $(type -p foo) ]]" to "type -p foo > /dev/null". In fact this seems to be best in two separate patches for me. 1) [ $(type -p foo) ] -> type -p foo > /dev/null 2) [ "$(type -t foo)" = "bar" ] -> [[ $(type -t foo) == "bar" ]] Allan
A new function, check_cmd, relies on type -p's return value instead of a string check. And gettext was previously being tested with type -t, which was inconsistent with the rest of the tests pertaining commands that aren't expected to be functions nor builtins. Signed-off-by: Andres P <aepd87@gmail.com> --- i don't think the inline comments i added are necessary, since the function is pretty explicit scripts/makepkg.sh.in | 39 ++++++++++++++++++++++++++------------- 1 files changed, 26 insertions(+), 13 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8c0da8b..650358b 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -193,6 +193,19 @@ get_url() { echo "${1#*::}" } + +## +# Checks to see if command is present in PATH +# +# usage : check_cmd( $command ) +# return : 0 - found +# 1 - not found +## +check_cmd() { + type -p -- "$1" >/dev/null +} + + ## # Checks to see if options are present in makepkg.conf or PKGBUILD; # PKGBUILD options always take precedence. @@ -345,7 +358,7 @@ download_file() { run_pacman() { local ret=0 if (( ! ASROOT )) && [[ $1 != "-T" && $1 != "-Qq" ]]; then - if [ "$(type -p sudo)" ] && sudo -l $PACMAN &>/dev/null; then + if check_cmd sudo && sudo -l $PACMAN &>/dev/null; then sudo $PACMAN $PACMAN_OPTS "$@" || ret=$? else su -c "$PACMAN $PACMAN_OPTS $*" || ret=$? @@ -519,7 +532,7 @@ generate_checksums() { msg "$(gettext "Generating checksums for source files...")" plain "" - if [ ! $(type -p openssl) ]; then + if ! check_cmd openssl; then error "$(gettext "Cannot find openssl.")" exit 1 # $E_MISSING_PROGRAM fi @@ -580,7 +593,7 @@ generate_checksums() { check_checksums() { (( ! ${#source[@]} )) && return 0 - if [ ! $(type -p openssl) ]; then + if ! check_cmd openssl; then error "$(gettext "Cannot find openssl.")" exit 1 # $E_MISSING_PROGRAM fi @@ -1333,27 +1346,27 @@ devel_check() { # Also do a brief check to make sure we have the VCS tool available. oldpkgver=$pkgver if [[ -n ${_darcstrunk} && -n ${_darcsmod} ]] ; then - [ $(type -p darcs) ] || return 0 + check_cmd darcs || return 0 msg "$(gettext "Determining latest darcs revision...")" newpkgver=$(date +%Y%m%d) elif [[ -n ${_cvsroot} && -n ${_cvsmod} ]] ; then - [ $(type -p cvs) ] || return 0 + check_cmd cvs || return 0 msg "$(gettext "Determining latest cvs revision...")" newpkgver=$(date +%Y%m%d) elif [[ -n ${_gitroot} && -n ${_gitname} ]] ; then - [ $(type -p git) ] || return 0 + check_cmd git || return 0 msg "$(gettext "Determining latest git revision...")" newpkgver=$(date +%Y%m%d) elif [[ -n ${_svntrunk} && -n ${_svnmod} ]] ; then - [ $(type -p svn) ] || return 0 + check_cmd 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} && -n ${_bzrmod} ]] ; then - [ $(type -p bzr) ] || return 0 + check_cmd bzr || return 0 msg "$(gettext "Determining latest bzr revision...")" newpkgver=$(bzr revno ${_bzrtrunk}) elif [[ -n ${_hgroot} && -n ${_hgrepo} ]] ; then - [ $(type -p hg) ] || return 0 + check_cmd hg || return 0 msg "$(gettext "Determining latest hg revision...")" if [[ -d ./src/$_hgrepo ]] ; then cd ./src/$_hgrepo @@ -1555,7 +1568,7 @@ There is NO WARRANTY, to the extent permitted by law.\n")" # PROGRAM START # determine whether we have gettext; make it a no-op if we do not -if [ ! $(type -t gettext) ]; then +if ! check_cmd gettext; then gettext() { echo "$@" } @@ -1729,7 +1742,7 @@ if (( ! INFAKEROOT )); then plain "$(gettext "Please rerun makepkg without the --asroot flag.")" exit 1 # $E_USER_ABORT elif [[ $(check_buildenv fakeroot) = "y" ]] && (( EUID > 0 )); then - if [ ! $(type -p fakeroot) ]; then + if ! check_cmd 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 @@ -1749,7 +1762,7 @@ fi # check for sudo if we will need it during makepkg execution if (( ! ( ASROOT || INFAKEROOT ) && ( DEP_BIN || RMDEPS || INSTALL ) )); then - if [ ! "$(type -p sudo)" ]; then + if ! check_cmd sudo; then warning "$(gettext "Sudo can not be found. Will use su to acquire root privileges.")" fi fi @@ -1921,7 +1934,7 @@ if (( NODEPS || ( (NOBUILD || REPKG) && !DEP_BIN ) )); then if (( NODEPS || ( REPKG && PKGFUNC ) )); then warning "$(gettext "Skipping dependency checks.")" fi -elif [ $(type -p "${PACMAN%% *}") ]; then +elif check_cmd "${PACMAN%% *}"; then if (( RMDEPS )); then original_pkglist=($(run_pacman -Qq | sort)) # required by remove_dep fi -- 1.7.1
On 27/05/10 02:05, Andres P wrote:
A new function, check_cmd, relies on type -p's return value instead of a string check.
And gettext was previously being tested with type -t, which was inconsistent with the rest of the tests pertaining commands that aren't expected to be functions nor builtins.
Signed-off-by: Andres P<aepd87@gmail.com> ---
i don't think the inline comments i added are necessary, since the function is pretty explicit
scripts/makepkg.sh.in | 39 ++++++++++++++++++++++++++------------- 1 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8c0da8b..650358b 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -193,6 +193,19 @@ get_url() { echo "${1#*::}" }
+ +## +# Checks to see if command is present in PATH +# +# usage : check_cmd( $command ) +# return : 0 - found +# 1 - not found +## +check_cmd() { + type -p -- "$1">/dev/null +} +
I do not see the need to split this out into a function. It is a single line that is not particularly complex and that is very unlikely to ever change. I'd much prefer this inline. Allan
Determining if a command is in PATH now relies on type -p's return value instead of a string check. And gettext was previously being checked with type -t, which is inconsistent with the rest of the tests pertaining commands that aren't expected to be functions nor builtins. Signed-off-by: Andres P <aepd87@gmail.com> --- this is a resubmit v2: inline function scripts/makepkg.sh.in | 26 +++++++++++++------------- 1 files changed, 13 insertions(+), 13 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8c0da8b..4287fad 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -345,7 +345,7 @@ download_file() { run_pacman() { local ret=0 if (( ! ASROOT )) && [[ $1 != "-T" && $1 != "-Qq" ]]; then - if [ "$(type -p sudo)" ] && sudo -l $PACMAN &>/dev/null; then + if type -p sudo >/dev/null && sudo -l $PACMAN &>/dev/null; then sudo $PACMAN $PACMAN_OPTS "$@" || ret=$? else su -c "$PACMAN $PACMAN_OPTS $*" || ret=$? @@ -519,7 +519,7 @@ generate_checksums() { msg "$(gettext "Generating checksums for source files...")" plain "" - if [ ! $(type -p openssl) ]; then + if ! type -p openssl >/dev/null; then error "$(gettext "Cannot find openssl.")" exit 1 # $E_MISSING_PROGRAM fi @@ -580,7 +580,7 @@ generate_checksums() { check_checksums() { (( ! ${#source[@]} )) && return 0 - if [ ! $(type -p openssl) ]; then + if ! type -p openssl >/dev/null; then error "$(gettext "Cannot find openssl.")" exit 1 # $E_MISSING_PROGRAM fi @@ -1333,27 +1333,27 @@ devel_check() { # Also do a brief check to make sure we have the VCS tool available. oldpkgver=$pkgver if [[ -n ${_darcstrunk} && -n ${_darcsmod} ]] ; then - [ $(type -p darcs) ] || return 0 + type -p darcs >/dev/null || return 0 msg "$(gettext "Determining latest darcs revision...")" newpkgver=$(date +%Y%m%d) elif [[ -n ${_cvsroot} && -n ${_cvsmod} ]] ; then - [ $(type -p cvs) ] || return 0 + type -p cvs >/dev/null || return 0 msg "$(gettext "Determining latest cvs revision...")" newpkgver=$(date +%Y%m%d) elif [[ -n ${_gitroot} && -n ${_gitname} ]] ; then - [ $(type -p git) ] || return 0 + type -p git >/dev/null || return 0 msg "$(gettext "Determining latest git revision...")" newpkgver=$(date +%Y%m%d) elif [[ -n ${_svntrunk} && -n ${_svnmod} ]] ; then - [ $(type -p svn) ] || return 0 + type -p svn >/dev/null || 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} && -n ${_bzrmod} ]] ; then - [ $(type -p bzr) ] || return 0 + type -p bzr >/dev/null || return 0 msg "$(gettext "Determining latest bzr revision...")" newpkgver=$(bzr revno ${_bzrtrunk}) elif [[ -n ${_hgroot} && -n ${_hgrepo} ]] ; then - [ $(type -p hg) ] || return 0 + type -p hg >/dev/null || return 0 msg "$(gettext "Determining latest hg revision...")" if [[ -d ./src/$_hgrepo ]] ; then cd ./src/$_hgrepo @@ -1555,7 +1555,7 @@ There is NO WARRANTY, to the extent permitted by law.\n")" # PROGRAM START # determine whether we have gettext; make it a no-op if we do not -if [ ! $(type -t gettext) ]; then +if ! type -p gettext >/dev/null; then gettext() { echo "$@" } @@ -1729,7 +1729,7 @@ if (( ! INFAKEROOT )); then plain "$(gettext "Please rerun makepkg without the --asroot flag.")" exit 1 # $E_USER_ABORT elif [[ $(check_buildenv fakeroot) = "y" ]] && (( EUID > 0 )); then - if [ ! $(type -p fakeroot) ]; then + if ! type -p fakeroot >/dev/null; then error "$(gettext "Fakeroot must be installed if using the 'fakeroot' option")" plain "$(gettext "in the BUILDENV array in %s.")" "$MAKEPKG_CONF" exit 1 @@ -1749,7 +1749,7 @@ fi # check for sudo if we will need it during makepkg execution if (( ! ( ASROOT || INFAKEROOT ) && ( DEP_BIN || RMDEPS || INSTALL ) )); then - if [ ! "$(type -p sudo)" ]; then + if ! type -p sudo >/dev/null; then warning "$(gettext "Sudo can not be found. Will use su to acquire root privileges.")" fi fi @@ -1921,7 +1921,7 @@ if (( NODEPS || ( (NOBUILD || REPKG) && !DEP_BIN ) )); then if (( NODEPS || ( REPKG && PKGFUNC ) )); then warning "$(gettext "Skipping dependency checks.")" fi -elif [ $(type -p "${PACMAN%% *}") ]; then +elif type -p "${PACMAN%% >/dev/null *}"; then if (( RMDEPS )); then original_pkglist=($(run_pacman -Qq | sort)) # required by remove_dep fi -- 1.7.1
On 27.05.2010 16:46, Andres P wrote:
@@ -1921,7 +1921,7 @@ if (( NODEPS || ( (NOBUILD || REPKG) && !DEP_BIN ) )); then if (( NODEPS || ( REPKG && PKGFUNC ) )); then warning "$(gettext "Skipping dependency checks.")" fi -elif [ $(type -p "${PACMAN%% *}") ]; then +elif type -p "${PACMAN%% >/dev/null *}"; then if (( RMDEPS )); then original_pkglist=($(run_pacman -Qq | sort)) # required by remove_dep fi
This line needs to be fixed before applying it.
On Thu, May 27, 2010 at 10:27 AM, Cedric Staniewski <cedric@gmx.ca> wrote:
On 27.05.2010 16:46, Andres P wrote:
@@ -1921,7 +1921,7 @@ if (( NODEPS || ( (NOBUILD || REPKG) && !DEP_BIN ) )); then if (( NODEPS || ( REPKG && PKGFUNC ) )); then warning "$(gettext "Skipping dependency checks.")" fi -elif [ $(type -p "${PACMAN%% *}") ]; then +elif type -p "${PACMAN%% >/dev/null *}"; then if (( RMDEPS )); then original_pkglist=($(run_pacman -Qq | sort)) # required by remove_dep fi
This line needs to be fixed before applying it.
thats what i get for patching with sed :p thanks for noticing, ill send a new patch soon -- Andres P
Rely on type -p's return value instead of a string check. And gettext was previously being checked with type -t, which was inconsistent with the rest of the tests pertaining commands that aren't expected to be functions nor builtins. Signed-off-by: Andres P <aepd87@gmail.com> --- this is a resubmit v2.1: correct -1749,7 +1749,7-ish syntax err (thanks cedric) v2: inline function scripts/makepkg.sh.in | 26 +++++++++++++------------- 1 files changed, 13 insertions(+), 13 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 76b6183..a2ac131 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -345,7 +345,7 @@ download_file() { run_pacman() { local ret=0 if (( ! ASROOT )) && [[ $1 != "-T" && $1 != "-Qq" ]]; then - if [ "$(type -p sudo)" ] && sudo -l $PACMAN &>/dev/null; then + if type -p sudo >/dev/null && sudo -l $PACMAN &>/dev/null; then sudo $PACMAN $PACMAN_OPTS "$@" || ret=$? else su -c "$PACMAN $PACMAN_OPTS $*" || ret=$? @@ -519,7 +519,7 @@ generate_checksums() { msg "$(gettext "Generating checksums for source files...")" plain "" - if [ ! $(type -p openssl) ]; then + if ! type -p openssl >/dev/null; then error "$(gettext "Cannot find openssl.")" exit 1 # $E_MISSING_PROGRAM fi @@ -580,7 +580,7 @@ generate_checksums() { check_checksums() { (( ! ${#source[@]} )) && return 0 - if [ ! $(type -p openssl) ]; then + if ! type -p openssl >/dev/null; then error "$(gettext "Cannot find openssl.")" exit 1 # $E_MISSING_PROGRAM fi @@ -1310,27 +1310,27 @@ devel_check() { # Also do a brief check to make sure we have the VCS tool available. oldpkgver=$pkgver if [[ -n ${_darcstrunk} && -n ${_darcsmod} ]] ; then - [ $(type -p darcs) ] || return 0 + type -p darcs >/dev/null || return 0 msg "$(gettext "Determining latest darcs revision...")" newpkgver=$(date +%Y%m%d) elif [[ -n ${_cvsroot} && -n ${_cvsmod} ]] ; then - [ $(type -p cvs) ] || return 0 + type -p cvs >/dev/null || return 0 msg "$(gettext "Determining latest cvs revision...")" newpkgver=$(date +%Y%m%d) elif [[ -n ${_gitroot} && -n ${_gitname} ]] ; then - [ $(type -p git) ] || return 0 + type -p git >/dev/null || return 0 msg "$(gettext "Determining latest git revision...")" newpkgver=$(date +%Y%m%d) elif [[ -n ${_svntrunk} && -n ${_svnmod} ]] ; then - [ $(type -p svn) ] || return 0 + type -p svn >/dev/null || 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} && -n ${_bzrmod} ]] ; then - [ $(type -p bzr) ] || return 0 + type -p bzr >/dev/null || return 0 msg "$(gettext "Determining latest bzr revision...")" newpkgver=$(bzr revno ${_bzrtrunk}) elif [[ -n ${_hgroot} && -n ${_hgrepo} ]] ; then - [ $(type -p hg) ] || return 0 + type -p hg >/dev/null || return 0 msg "$(gettext "Determining latest hg revision...")" if [[ -d ./src/$_hgrepo ]] ; then cd ./src/$_hgrepo @@ -1532,7 +1532,7 @@ There is NO WARRANTY, to the extent permitted by law.\n")" # PROGRAM START # determine whether we have gettext; make it a no-op if we do not -if [ ! $(type -t gettext) ]; then +if ! type -p gettext >/dev/null; then gettext() { echo "$@" } @@ -1706,7 +1706,7 @@ if (( ! INFAKEROOT )); then plain "$(gettext "Please rerun makepkg without the --asroot flag.")" exit 1 # $E_USER_ABORT elif [[ $(check_buildenv fakeroot) = "y" ]] && (( EUID > 0 )); then - if [ ! $(type -p fakeroot) ]; then + if ! type -p fakeroot >/dev/null; then error "$(gettext "Fakeroot must be installed if using the 'fakeroot' option")" plain "$(gettext "in the BUILDENV array in %s.")" "$MAKEPKG_CONF" exit 1 @@ -1726,7 +1726,7 @@ fi # check for sudo if we will need it during makepkg execution if (( ! ( ASROOT || INFAKEROOT ) && ( DEP_BIN || RMDEPS || INSTALL ) )); then - if [ ! "$(type -p sudo)" ]; then + if ! type -p sudo >/dev/null; then warning "$(gettext "Sudo can not be found. Will use su to acquire root privileges.")" fi fi @@ -1898,7 +1898,7 @@ if (( NODEPS || ( (NOBUILD || REPKG) && !DEP_BIN ) )); then if (( NODEPS || ( REPKG && PKGFUNC ) )); then warning "$(gettext "Skipping dependency checks.")" fi -elif [ $(type -p "${PACMAN%% *}") ]; then +elif type -p "${PACMAN%% *}" >/dev/null; then if (( RMDEPS )); then original_pkglist=($(run_pacman -Qq | sort)) # required by remove_dep fi -- 1.7.1
On 28/05/10 01:06, Andres P wrote:
Rely on type -p's return value instead of a string check.
And gettext was previously being checked with type -t, which was inconsistent with the rest of the tests pertaining commands that aren't expected to be functions nor builtins.
Signed-off-by: Andres P<aepd87@gmail.com> --- this is a resubmit
v2.1: correct -1749,7 +1749,7-ish syntax err (thanks cedric)
v2: inline function
scripts/makepkg.sh.in | 26 +++++++++++++------------- 1 files changed, 13 insertions(+), 13 deletions(-)
This version is fine to be applied. However, we are now close to releasing 3.4 so not much apart from bug fixes will make it to master until that is done. Pushed to my post-3.4 branch so it does not get lost in the meantime. Allan
Change all instances of the (test) [ builtin to the [[ keyword. Signed-off-by: Andres P <aepd87@gmail.com> --- v2 remove comment regarding bash 4.0 hack scripts/makepkg.sh.in | 10 ++++------ 1 files changed, 4 insertions(+), 6 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8c0da8b..773d779 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1296,7 +1296,7 @@ check_sanity() { if (( ${#pkgname[@]} > 1 )); then for pkg in ${pkgname[@]}; do - if [ "$(type -t package_${pkg})" != "function" ]; then + if [[ $(type -t package_${pkg}) != "function" ]]; then error "$(gettext "missing package function for split package '%s'")" "$pkg" return 1 fi @@ -1805,14 +1805,12 @@ if (( ${#pkgname[@]} > 1 )); then fi # test for available PKGBUILD functions -# The exclamation mark is required here to avoid triggering the ERR trap when -# a tested function does not exist. -if [[ $(! type -t build) = "function" ]]; then +if [[ $(type -t build) = "function" ]]; then BUILDFUNC=1 fi -if [ "$(type -t package)" = "function" ]; then +if [[ $(type -t package) = "function" ]]; then PKGFUNC=1 -elif [ $SPLITPKG -eq 0 -a "$(type -t package_${pkgname})" = "function" ]; then +elif [[ $SPLITPKG -eq 0 && $(type -t package_${pkgname}) = "function" ]]; then SPLITPKG=1 fi -- 1.7.1
On 27/05/10 02:05, Andres P wrote:
Change all instances of the (test) [ builtin to the [[ keyword.
Signed-off-by: Andres P<aepd87@gmail.com> ---
v2 remove comment regarding bash 4.0 hack
scripts/makepkg.sh.in | 10 ++++------ 1 files changed, 4 insertions(+), 6 deletions(-)
Signoff. On my "post-3.4" branch. Allan
On Tue, May 25, 2010 at 03:36:41PM -0430, Andres P wrote:
`type -p foo` has a return val, so consider this: $ time for i in {1..1000}; do [[ $(type -p sh) ]]; done real 0m1.564s user 0m0.160s sys 0m0.337s
$ time for i in {1..1000}; do type -p sh &>/dev/null; done real 0m0.166s user 0m0.060s sys 0m0.103s
I should have brought this up sooner: check_fn() { # if [[ $(type -t "$1") = "function" ]]; then if declare -f "$1" >/dev/null; then echo true else echo false fi } build() { :; } check_fn build # true unset build check_fn build # false check_fn bash # false Assuming there's interest, I'll write a patch once master i open again. Andres P
On 29/05/10 05:16, Andres P wrote:
On Tue, May 25, 2010 at 03:36:41PM -0430, Andres P wrote:
`type -p foo` has a return val, so consider this: $ time for i in {1..1000}; do [[ $(type -p sh) ]]; done real 0m1.564s user 0m0.160s sys 0m0.337s
$ time for i in {1..1000}; do type -p sh&>/dev/null; done real 0m0.166s user 0m0.060s sys 0m0.103s
I should have brought this up sooner:
check_fn() { # if [[ $(type -t "$1") = "function" ]]; then if declare -f "$1">/dev/null; then echo true else echo false fi }
build() { :; }
check_fn build # true
unset build
check_fn build # false
check_fn bash # false
Assuming there's interest, I'll write a patch once master i open again.
Sure. It is much faster to check for functions that way. Feel free to base it of my post-3.4 branch is you want to do this pre pacman-3.4 release. Allan
participants (6)
-
Allan McRae
-
Andres P
-
Andres Perera
-
Cedric Staniewski
-
Dan McGee
-
Xavier Chantry