On Dec 4, 2007 6:18 PM, Dan McGee wrote:

On Dec 4, 2007 6:06 PM, Aaron Griffin wrote:

...

On Dec 4, 2007 5:54 PM, Allan McRae wrote:

...

Aaron Griffin wrote:

...

Besides what Roman mentioned, I noticed you used the backtick inline execution syntax - we like to frown on that around here. Could you please use $() instead?

Fixed. I didn't know about that syntax - how is it better?

Well, first off, backtick expansions is officially deprecated if I remember correctly, but at the rate these standards move it will be some time before it dies. In addition to hat, $() is much cleaner when it comes to parsing and nesting. It's easier to match a pair of characters than two of the same characters. Take this for example:

`foo -c `bar -x`` $(foo -c $(bar -x))

The first one might not even parse right in the first place.

...

New patch attached.

Danke! It looks good to me. We don't have a testsuite for makepkg like we do for pacman, so do you happen to know any packages, offhand, that suffer from the hardlink issue, so that I can test this?

I'm making a few changes to the patch locally, and I'll send it here if it works right.

-Dan

I've pasted the new patch below, as well as a sample PKGBUILD. Some comments inlined where I changed things. diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 953bda2..0ef0e52 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -722,18 +722,34 @@ tidy_install() { msg2 "$(gettext "Compressing man pages...")" - local manpage ext file link - find {usr{,/local},opt/*}/man -type f 2>/dev/null | while read manpage ; do - ext="${manpage##*.}" - file="${manpage##*/}" - if [ "$ext" != "gz" -a "$ext" != "bz2" ]; then - # update symlinks to this manpage - find {usr{,/local},opt/*}/man -lname "$file" 2>/dev/null | while read link ; do - rm -f "$link" - ln -sf "${file}.gz" "${link}.gz" - done - # compress the original - gzip -9 "$manpage" + local manpage mandirs ext file link hardlinks hl + mandirs="usr/man usr/local/man usr/share/man opt/*/man" I added usr/share/man to the list as it is supposed to be the standard location for manpages (even though we move them above). BTW- why do we move them, Aaron? This uncovered another bug which I will explain below. + find ${mandirs} -type f 2>/dev/null | while read manpage ; do + # check file still exists (potentially compressed with hard link) + if [ -f ${manpage} ]; then + ext="${manpage##*.}" + file="${manpage##*/}" + if [ "$ext" != "gz" -a "$ext" != "bz2" ]; then + # update symlinks to this manpage + find ${mandirs} -lname "$file" 2>/dev/null | while read link ; do + rm -f "$link" + ln -sf "${file}.gz" "${link}.gz" + done + # find hard links and remove them + # the '|| true' part keeps the script from bailing if find returned an + # error, such as when one of the man directories doesn't exist + hardlinks="$(find ${mandirs} \! -name "$file" -samefile "$manpage" 2>/dev/null)" || true I didn't like having to parse ls output (and start another process) just to get an inode number, so I changed it to use the -samefile test of find. When doing this, I realized that because we execute our makepkg script with the -e option, returning an error code causes hardlinks to get set to nothing, and thus we don't do any file removal or creation. Thus the '|| true' and my comment above, which is similar to a construct used during the getopts call (Andrew, I believe you did that). + for hl in ${hardlinks}; do + rm -f "${hl}"; + done No need for the if statements, as a loop on nothing does nothing wrong. + # compress the original + gzip -9 "$manpage" + # recreate hard links removed earlier + for hl in ${hardlinks}; do + ln "${manpage}.gz" "${hl}.gz" + chmod 644 ${hl}.gz + done + fi fi done $ cat PKGBUILD pkgname=hardlink_test pkgver=0.1 pkgrel=1 arch=('i686' 'x86_64') build() { cd "$pkgdir" for dir in usr usr/local usr/share opt/test; do mkdir -pv $dir/man/man1 echo "test manpage compression" > $dir/man/man1/foo.1 ln -v $dir/man/man1/foo.1 $dir/man/man1/bar.1 ln -v $dir/man/man1/foo.1 $dir/man/man1/xyz.1 done }