[pacman-dev] [PATCH] [makepkg] use double brackets
Signed-off-by: Andres P
Looked like slackware's makepkg
Signed-off-by: Andres P
On 22/05/10 15:06, Andres P wrote:
Looked like slackware's makepkg
Signed-off-by: Andres P
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
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
On Sat, May 22, 2010 at 1:48 AM, Allan McRae
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
On Sat, May 22, 2010 at 5:14 PM, Dan McGee
wrote: On Sat, May 22, 2010 at 1:48 AM, Allan McRae
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
wrote: On Sat, May 22, 2010 at 1:48 AM, Allan McRae
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
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
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
On Sat, May 22, 2010 at 2:18 AM, Allan McRae
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
On Tue, May 25, 2010 at 10:06 PM, Andres P
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
wrote: On Tue, May 25, 2010 at 10:06 PM, Andres P
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
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
--- 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
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
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
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
--- 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
On 27/05/10 02:05, Andres P wrote:
Change all instances of the (test) [ builtin to the [[ keyword.
Signed-off-by: Andres P
--- 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