[pacman-dev] [PATCH] [makepkg] New patch for split debug support, using generic split packaging mechanism
Dave Reisner
d at falconindy.com
Sat Aug 20 09:33:23 EDT 2011
On Sat, Aug 20, 2011 at 04:07:24PM +0300, Vitaliy Gorbunov wrote:
> I have read submitting_patches.txt and created git formatted patch. See it
> below.
>
> Some comments about previous reviewing:
>
> > create_package() {
> >
> > + if (( SKIP_DEBUG_PACKAGE )); then
> >
> > + warning "$(gettext "No .debug files for debug package found,
> >
> > skipping...")"
> > > + SKIP_DEBUG_PACKAGE=0
> >
> Why are we resetting the variable? Do we do this once per regular
> >
> package function? This seems misplaced, even if so.
> >
> SKIPDEBUGPACKAGE is needed to determine that debug files for package was not
> created (package has 'any' architecture or package was compiled without
> debug symbols). And we need to reset it, because the next package after
> debug will be regular package. I can't think of simpler way to do it.
>
> > + restore_package_variables "dbg"
> > > + pkgdesc="$pkgdesc (debugging symbols)"
> > > + depends=("${pkgname%-dbg}=$
> > pkgver-$pkgrel")
> > > + groups=('debug' ${groups[@]/%/-debug})
> > This seems very magic and arbitrary.
> >
> Why? If group consist some package, then ${group}-debug will consist
> appropriate
> debug package. It's seems fine for me.
>
>
> > > + if readelf -S $binary | grep "\.gnu_debuglink"
> > &>/dev/null;
> >
> This is very Linux specific. Not sure what we think about this.
> >
> What do you mean? Why this is Linux-specific?
>
readelf is Linux specific, and you're assuming glibc/gcc. There's not
really a portable way to read data out of executables given the array of
formats for the platforms we support: PE, COFF, a.out, ELF, Mach-O...
You're not going to find readelf in normal usage on cygwin or OSX.
There's some other blockers I've commented on inline below. I sort of
gave up with commenting on style because there's too much of it to
cover and I start sounding like a broken record.
I'll just leave this here: http://mywiki.wooledge.org/BashGuide
d
>
> This add support for split debug packages. The main idea is that debug
> packages will be added to pkgname array and for each of it
> package_${pkgname}-debug function will be created. So it treated like usual
> split packages.
>
> Signed-off-by: Vitaly Gorbunov <vit.gorbunov at gmail.com>
> ---
> scripts/makepkg.sh.in | 85
> +++++++++++++++++++++++++++++++++++++++++++-----
> 1 files changed, 76 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index d0a514a..08c4a09 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -72,6 +72,8 @@ BUILDFUNC=0
> CHECKFUNC=0
> PKGFUNC=0
> SPLITPKG=0
> +SPLITDEBUG=0
> +SKIPDEBUGPACKAGE=0
> PKGLIST=()
> SIGNPKG=''
>
> @@ -973,7 +975,7 @@ tidy_install() {
> done
> fi
>
> - if [[ $(check_option zipman) = "y" && -n ${MAN_DIRS[*]} ]]; then
> + if [[ $(check_option zipman) != "n" && -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 |
> @@ -1009,21 +1011,52 @@ tidy_install() {
> done
> fi
>
> + if [ "$(check_option splitdebug)" = "y" ] || (( SPLITDEBUG )); then
> + local is_splitdebug=1
> + local dbgdir="$pkgdir-debug"
> + backup_package_variables "debug"
> + fi
> if [[ $(check_option strip) = y ]]; then
> msg2 "$(gettext "Stripping unneeded symbols from binaries and
> libraries...")"
> # make sure library stripping variables are defined to prevent
> excess stripping
> [[ -z ${STRIP_SHARED+x} ]] && STRIP_SHARED="-S"
> [[ -z ${STRIP_STATIC+x} ]] && STRIP_STATIC="-S"
> - local binary
> + local binary hardlink strip_opt
> find . -type f -perm -u+w 2>/dev/null | while read binary ; do
> case "$(file -bi "$binary")" in
> *application/x-sharedlib*) # Libraries (.so)
> - strip $STRIP_SHARED "$binary";;
> + strip_opt=$STRIP_SHARED;;
> *application/x-archive*) # Libraries (.a)
> - strip $STRIP_STATIC "$binary";;
> + strip_opt=$STRIP_STATIC;;
> *application/x-executable*) # Binaries
> - strip $STRIP_BINARIES "$binary";;
> + strip_opt=$STRIP_BINARIES;;
> + *) continue ;;
> esac
> + # Strip symbols
> + if (( is_splitdebug )); then
> + # Create directory for the symbol file
> + mkdir -p "${dbgdir}/usr/lib/debug/${binary%/*}"
> + if readelf -S $binary | grep "\.gnu_debuglink" &>/dev/null;
> then
> + # An already stripped binary is a hard link
> + find ${STRIP_DIRS[@]} -samefile "$binary" 2>/dev/null |
> while read hardlink ; do
Quote the array. Null delimited output on find...
If you're going to loop over find's output, please use -print0 on the
find side and -d '' on the read side. There's no reason not to be safe
about this.
> + # Is the symbol file for the hardlink in the right
> dir ?
> + if [ -e
> "${dbgdir}/usr/lib/debug/${hardlink#/}.debug" ]; then
style! we don't use [ ] anywhere else in makepkg.
> + if [ "${hardlink%/*}" != "${binary%/*}" ]; then
> + ln
> "${dbgdir}/usr/lib/debug/${hardlink#/}.debug"
> "${dbgdir}/usr/lib/debug/${binary%/*}"
> + break
> + fi
> + fi
> + done
> + else
> + # Strip the file
> + objcopy --only-keep-debug "$binary"
> "${dbgdir}/usr/lib/debug/${binary#/}.debug"
> + strip $strip_opts "$binary"
> + objcopy
> --add-gnu-debuglink="/usr/lib/debug/${binary#/}.debug" "$binary"
objcopy definitely doesn't portably support the --add-gnu-debuglink
flag...
> + chmod 644 "${dbgdir}/usr/lib/debug/${binary#/}.debug"
> + fi
> + else
> + strip $strip_opts "$binary"
> + fi
> done
> fi
>
> @@ -1332,6 +1365,33 @@ create_signature() {
> fi
> }
>
> +add_split_debug_packages(){
> + [ ${#pkgname[@]} = 1 ] && eval "function package_${pkgname}() {
> package;}"
We don't prefix with function.
> +
> + local pkg pkgname_with_debug
> + for pkg in ${pkgname[@]}
> + do
style. We always have 'do' on the same line as 'for'. quoting, too.
> + pkgname_with_debug=( ${pkgname_with_debug[@]} ${pkg}
> ${pkg/%/-debug} )
> + eval "function package_${pkg}-debug() { package_debug; }"
> + done
> + pkgname=( ${pkgname_with_debug[@]} )
more quoting...
> +}
> +
> +package_debug(){
> + check_debug_files=`find $pkgdir/ -name "*.debug"`
> + if [ -z "$check_debug_files" ]; then
> + SKIPDEBUGPACKAGE=1
> + return
> + fi
> +
> + restore_package_variables "debug"
> + pkgdesc="$pkgdesc (debugging symbols)"
> + depends=("${pkgname%-debug}=$pkgver-$pkgrel")
> + groups=('debug' ${groups[@]/%/-debug})
> + options=(!strip !splitdebug !zipman !purge !emptydirs !upx)
> + unset optdepends provides conflicts replaces backup install changelog
> +}
> +
> create_srcpackage() {
> msg "$(gettext "Creating source package...")"
> local srclinks="$(mktemp -d "$startdir"/srclinks.XXXXXXXXX)"
> @@ -1776,16 +1836,18 @@ devel_update() {
>
> backup_package_variables() {
> local var
> + local suffix=$1
> for var in ${splitpkg_overrides[@]}; do
> - local indirect="${var}_backup"
> + local indirect="${var}_backup${suffix}"
> eval "${indirect}=(\"\${$var[@]}\")"
> done
> }
>
> restore_package_variables() {
> local var
> + local suffix=$1
> for var in ${splitpkg_overrides[@]}; do
> - local indirect="${var}_backup"
> + local indirect="${var}_backup${suffix}"
> if [[ -n ${!indirect} ]]; then
> eval "${var}=(\"\${$indirect[@]}\")"
> else
> @@ -1849,6 +1911,7 @@ usage() {
> printf "$(gettext " -s, --syncdeps Install missing dependencies with
> %s")\n" "pacman"
> echo "$(gettext " -S, --source Generate a source-only tarball
> without downloaded sources")"
> echo "$(gettext " --allsource Generate a source-only tarball
> including downloaded sources")"
> + echo "$(gettext " -t, --splitdebug Split debugging symbols into
> separate package")"
> printf "$(gettext " --asroot Allow %s to run as root user")\n"
> "makepkg"
> printf "$(gettext " --check Run the %s function in the
> %s")\n" "check()" "$BUILDSCRIPT"
> printf "$(gettext " --config <file> Use an alternate config file
> (instead of '%s')")\n" "$confdir/makepkg.conf"
> @@ -1892,12 +1955,12 @@ fi
> ARGLIST=("$@")
>
> # Parse Command Line Options.
> -OPT_SHORT="AcdefFghiLmop:rRsSV"
> +OPT_SHORT="AcdefFghiLmop:rRsStV"
> OPT_LONG="allsource,asroot,ignorearch,check,clean,nodeps"
> OPT_LONG+=",noextract,force,forcever:,geninteg,help,holdver,skippgpcheck"
> OPT_LONG+=",install,key:,log,nocolor,nobuild,nocheck,nosign,pkg:,rmdeps"
> OPT_LONG+=",repackage,skipchecksums,skipinteg,skippgpcheck,sign,source,syncdeps"
> -OPT_LONG+=",version,config:"
> +OPT_LONG+=",splitdebug,version,config:"
>
> # Pacman Options
> OPT_LONG+=",noconfirm,noprogressbar"
> @@ -1945,6 +2008,7 @@ while true; do
> --sign) SIGNPKG='y' ;;
> -s|--syncdeps) DEP_BIN=1 ;;
> -S|--source) SOURCEONLY=1 ;;
> + -t|--splitdebug) SPLITDEBUG=1 ;;
>
> -h|--help) usage; exit 0 ;; # E_OK
> -V|--version) version; exit 0 ;; # E_OK
> @@ -2130,6 +2194,9 @@ check_software || exit 1
> devel_check
> devel_update
>
> +if [ "$(check_option splitdebug)" = "y" ] || (( SPLITDEBUG )); then
> + add_split_debug_packages
> +fi
> if (( ${#pkgname[@]} > 1 )); then
> SPLITPKG=1
> fi
> --
> 1.7.6
>
More information about the pacman-dev
mailing list