Re: [pacman-dev] Contributing BSD patches upstream
On Wed, Oct 6, 2010 at 6:38 AM, Nezmer <me@nezmer.info> wrote:
On Tue, Oct 05, 2010 at 07:56:53PM -0500, Dan McGee wrote:
On Tue, Oct 5, 2010 at 6:03 PM, Dan McGee <dpmcgee@gmail.com> wrote:
Hey,
I noticed you did some merges and updates on your pacman-bsd branch today (http://gitorious.org/pacman-bsd/pacman-bsd). Have you thought about trying to get some of this work into the main branch of code so you don't have to maintain as much? I know we've already taken in a few things, like the makepkg colors checking stuff, but it might be nice to ensure we are as portable as possible out of the box.
I CC-ed in Allan because a lot of these are touchups to makepkg. I know we may not seem to move quickly at times, but we do appreciate patches and contributions from others.
My quick review of the differences picked out these things as stuff that should/could be looked at to improve compatibility and correctness:
1. DBPath/CacheDir parsing in scripts/ and contrib/ 2. Fix bash regexes: =~ should never have RHS quotes 3. Hardcoded '.tar.gz' in some contrib/ scripts 4. contrib/, use configurable etc/ directory (sysconfdir I believe) 5. bsdtar binary location, don't assume it is on path? 6. Use of seq (not a bash builtin) 7. makepkg: strip binary location hardcoded, perhaps use $STRIP 8. makepkg: mknod vs. mkfifo 9. bash shebang in scripts
-Dan
Just saw this. A very good summary.
I'm welling to answer any questions or put together any patch you are interested to take.
I'd basically say anything that is in the above list is fair game for patches. #2 and #3 are pretty low-hanging, non-invasive stuff if you'd like somewhere to start. I think I just handled #6 as we only used it in one contrib script; patch going to the ML. This message is also going to the ML so we can maybe get some discussion going there. -Dan
This is not a bash builtin, so can potentially cause portability issues. Additionally, the use of it is completely unnecessary as it can all be done within bash (and done faster). $ time pactree xfwm4 >/dev/null (old version) real 0m3.245s $ time ./contrib/pactree xfwm4 >/dev/null (new version) real 0m3.042s Signed-off-by: Dan McGee <dan@archlinux.org> --- contrib/pactree | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/contrib/pactree b/contrib/pactree index 73bece3..cb719f3 100755 --- a/contrib/pactree +++ b/contrib/pactree @@ -130,9 +130,12 @@ _tree(){ # Generate the spacer spacer="" - for each in $(seq 1 $spaces); do + local count=0 + while [[ $count -lt $spaces ]]; do spacer="$spacer$separator" + count=$((count+1)) done + unset count spacer="$spacer$branch_tip" [ $silent -ne 1 ] && echo -e "$branch_color$spacer$leaf_color$pkg_name$provided" -- 1.7.3.1
On Thu, Oct 07, 2010 at 03:52:34PM -0500, Dan McGee wrote:
This is not a bash builtin, so can potentially cause portability issues. Additionally, the use of it is completely unnecessary as it can all be done within bash (and done faster).
$ time pactree xfwm4 >/dev/null (old version) real 0m3.245s
$ time ./contrib/pactree xfwm4 >/dev/null (new version) real 0m3.042s
Signed-off-by: Dan McGee <dan@archlinux.org> --- contrib/pactree | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/contrib/pactree b/contrib/pactree index 73bece3..cb719f3 100755 --- a/contrib/pactree +++ b/contrib/pactree @@ -130,9 +130,12 @@ _tree(){
# Generate the spacer spacer="" - for each in $(seq 1 $spaces); do + local count=0 + while [[ $count -lt $spaces ]]; do spacer="$spacer$separator" + count=$((count+1)) done + unset count spacer="$spacer$branch_tip"
[ $silent -ne 1 ] && echo -e "$branch_color$spacer$leaf_color$pkg_name$provided" -- 1.7.3.1
Can I suggest using a C style for loop instead? for (( count=0; count < spaces; count++ )); do # stuff... done unset count d
On Thu, Oct 7, 2010 at 4:03 PM, Dave Reisner <d@falconindy.com> wrote:
On Thu, Oct 07, 2010 at 03:52:34PM -0500, Dan McGee wrote:
This is not a bash builtin, so can potentially cause portability issues. Additionally, the use of it is completely unnecessary as it can all be done within bash (and done faster).
$ time pactree xfwm4 >/dev/null (old version) real 0m3.245s
$ time ./contrib/pactree xfwm4 >/dev/null (new version) real 0m3.042s
Signed-off-by: Dan McGee <dan@archlinux.org> --- contrib/pactree | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/contrib/pactree b/contrib/pactree index 73bece3..cb719f3 100755 --- a/contrib/pactree +++ b/contrib/pactree @@ -130,9 +130,12 @@ _tree(){
# Generate the spacer spacer="" - for each in $(seq 1 $spaces); do + local count=0 + while [[ $count -lt $spaces ]]; do spacer="$spacer$separator" + count=$((count+1)) done + unset count spacer="$spacer$branch_tip"
[ $silent -ne 1 ] && echo -e "$branch_color$spacer$leaf_color$pkg_name$provided" -- 1.7.3.1
Can I suggest using a C style for loop instead?
for (( count=0; count < spaces; count++ )); do # stuff... done unset count
Definitely, adjusted the patch. Thanks. -Dan
On Thu, Oct 7, 2010 at 16:52, Dan McGee <dan@archlinux.org> wrote:
This is not a bash builtin, so can potentially cause portability issues. Additionally, the use of it is completely unnecessary as it can all be done within bash (and done faster).
$ time pactree xfwm4 >/dev/null (old version) real 0m3.245s
$ time ./contrib/pactree xfwm4 >/dev/null (new version) real 0m3.042s
Signed-off-by: Dan McGee <dan@archlinux.org> --- If we accept a dependency on bash >= 4, we can further reduce that all to a brace expansion: daenyth@Bragi ~ $ echo {01..10} 01 02 03 04 05 06 07 08 09 10
On Thu, Oct 7, 2010 at 4:03 PM, Daenyth Blank <daenyth+arch@gmail.com> wrote:
On Thu, Oct 7, 2010 at 16:52, Dan McGee <dan@archlinux.org> wrote:
This is not a bash builtin, so can potentially cause portability issues. Additionally, the use of it is completely unnecessary as it can all be done within bash (and done faster).
$ time pactree xfwm4 >/dev/null (old version) real 0m3.245s
$ time ./contrib/pactree xfwm4 >/dev/null (new version) real 0m3.042s
Signed-off-by: Dan McGee <dan@archlinux.org> --- If we accept a dependency on bash >= 4, we can further reduce that all to a brace expansion: daenyth@Bragi ~ $ echo {01..10} 01 02 03 04 05 06 07 08 09 10
We don't. -Dan [1] http://projects.archlinux.org/pacman.git/commit/?id=c2cf6a14cf44400d0ef249b3... [2] http://projects.archlinux.org/pacman.git/commit/?id=8b23aa172f6229dd82c38170...
On Thu, Oct 7, 2010 at 3:46 PM, Dan McGee <dpmcgee@gmail.com> wrote:
On Wed, Oct 6, 2010 at 6:38 AM, Nezmer <me@nezmer.info> wrote:
On Tue, Oct 05, 2010 at 07:56:53PM -0500, Dan McGee wrote:
My quick review of the differences picked out these things as stuff that should/could be looked at to improve compatibility and correctness:
1. DBPath/CacheDir parsing in scripts/ and contrib/ 2. Fix bash regexes: =~ should never have RHS quotes 3. Hardcoded '.tar.gz' in some contrib/ scripts 4. contrib/, use configurable etc/ directory (sysconfdir I believe) 5. bsdtar binary location, don't assume it is on path? 6. Use of seq (not a bash builtin) 7. makepkg: strip binary location hardcoded, perhaps use $STRIP 8. makepkg: mknod vs. mkfifo 9. bash shebang in scripts
-Dan
Just saw this. A very good summary.
I'm welling to answer any questions or put together any patch you are interested to take.
I'd basically say anything that is in the above list is fair game for patches. #2 and #3 are pretty low-hanging, non-invasive stuff if you'd like somewhere to start.
I think I just handled #6 as we only used it in one contrib script; patch going to the ML. This message is also going to the ML so we can maybe get some discussion going there.
-Dan
OK, so we're down to this:
1. DBPath/CacheDir parsing in scripts/ and contrib/ 2. Fix bash regexes: =~ should never have RHS quotes To find them: $ git grep "=~.*[\"']"
3. Hardcoded '.tar.gz' in some contrib/ scripts Just this line in wget-xdelta? if ! [[ "$out_file" =~ "pkg.tar.gz" ]]; then
5. bsdtar binary location, don't assume it is on path? Maybe we do another AC_PATH_PROG() check? Not sure.
7. makepkg: strip binary location hardcoded, perhaps use $STRIP Ditto to the above thought.
-Dan
participants (4)
-
Daenyth Blank
-
Dan McGee
-
Dan McGee
-
Dave Reisner