[pacman-dev] [PATCH] [makepkg] New patch for split debug support, using generic split packaging mechanism
---------- Forwarded message ---------- From: Vitaliy Gorbunov <vit.gorbunov@gmail.com> Date: Wed, Aug 10, 2011 at 9:14 PM Subject: Re: [pacman-dev] [PATCH] [makepkg] New patch for split debug support, using generic split packaging mechanism To: Dan McGee <dpmcgee@gmail.com> Sorry:) Here it is. --- bin/makepkg 2011-08-10 19:37:37.000000000 +0300 +++ local/bin/makepkg4 2011-08-10 19:41:15.000000000 +0300 @@ -44,7 +44,7 @@ srcdir="$startdir/src" pkgdir="$startdir/pkg" -packaging_options=('strip' 'docs' 'libtool' 'emptydirs' 'zipman' 'purge') +packaging_options=('strip' 'splitdbg' 'docs' 'libtool' 'emptydirs' 'zipman' 'purge') other_options=('ccache' 'distcc' 'buildflags' 'makeflags') splitpkg_overrides=('pkgver' 'pkgrel' 'pkgdesc' 'arch' 'license' 'groups' \ 'depends' 'optdepends' 'provides' 'conflicts' 'replaces' \ @@ -74,6 +74,8 @@ CHECKFUNC=0 PKGFUNC=0 SPLITPKG=0 +SPLIT_DBG=0 +SKIP_DEBUG_PACKAGE=0 PKGLIST=() # Forces the pkgver of the current PKGBUILD. Used by the fakeroot call @@ -902,21 +904,49 @@ done fi + local is_splitdbg + [ "$(check_option splitdbg)" = "y" ] || (( SPLIT_DBG )) && is_splitdbg=1 + (( is_splitdbg )) && backup_package_variables "dbg" + + local dbgdir="$pkgdir-dbg" 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) - /usr/bin/strip $STRIP_SHARED "$binary";; + strip_opt=$STRIP_SHARED;; *application/x-archive*) # Libraries (.a) - /usr/bin/strip $STRIP_STATIC "$binary";; + strip_opt=$STRIP_STATIC;; *application/x-executable*) # Binaries - /usr/bin/strip $STRIP_BINARIES "$binary";; + strip_opt=$STRIP_BINARIES;; + *) continue ;; esac + # Strip symbols + if (( is_splitdbg )); 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 + # Is the symbol file for the hardlink in the right dir ? + [ -e "${dbgdir}/usr/lib/debug/${hardlink#/}.debug" ] && \ + [ "${hardlink%/*}" != "${binary%/*}" ] && \ + ln "${dbgdir}/usr/lib/debug/${hardlink#/}.debug" "${dbgdir}/usr/lib/debug/${binary%/*}" && break + done + else + # Strip the file + /usr/bin/objcopy --only-keep-debug "$binary" "${dbgdir}/usr/lib/debug/${binary#/}.debug" + /usr/bin/strip $strip_opts "$binary" + /usr/bin/objcopy --add-gnu-debuglink="${dbgdir}/usr/lib/debug/${binary#/}.debug" "$binary" + chmod 644 "${dbgdir}/usr/lib/debug/${binary#/}.debug" + fi + else + /usr/bin/strip $strip_opts "$binary" + fi done fi @@ -1008,6 +1038,12 @@ } create_package() { + if (( SKIP_DEBUG_PACKAGE )); then + warning "$(gettext "No .debug files for debug package found, skipping...")" + SKIP_DEBUG_PACKAGE=0 + return + fi + if [[ ! -d $pkgdir ]]; then error "$(gettext "Missing pkg/ directory.")" plain "$(gettext "Aborting...")" @@ -1096,6 +1132,38 @@ fi } +add_split_debug_packages(){ + [ ${#pkgname[@]} = 1 ] && eval "function package_${pkgname}() { package;}" + + local pkg pkgname_dbg + for pkg in ${pkgname[@]} + do + pkgname_dbg=( ${pkgname_dbg[@]} ${pkg} ${pkg/%/-dbg} ) + eval "function package_${pkg}-dbg() { package_dbg; }" + done + pkgname=( ${pkgname_dbg[@]} ) +} + +package_dbg(){ + check_dbg_files=`find $pkgdir/ -name "*.debug"` + if [ -z "$check_dbg_files" ]; then + SKIP_DEBUG_PACKAGE=1 + return + fi + + restore_package_variables "dbg" + pkgdesc="$pkgdesc (debugging symbols)" + depends=("${pkgname%-dbg}=$pkgver-$pkgrel") + groups=('debug' ${groups[@]/%/-debug}) + optdepends=() + replaces=() + provides=() + conflicts=() + backup=() + options=(!strip !splitdbg docs !zipman !purge libtool !emptydirs) + unset install +} + create_srcpackage() { cd "$startdir" @@ -1423,17 +1491,19 @@ } backup_package_variables() { - local var + local var suffix + 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 var suffix + 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 @@ -1582,6 +1652,7 @@ echo "$(gettext " -r, --rmdeps Remove installed dependencies after a successful build")" echo "$(gettext " -R, --repackage Repackage contents of the package without rebuilding")" echo "$(gettext " -s, --syncdeps Install missing dependencies with pacman")" + echo "$(gettext " -t, --splitdbg Split debugging symbols into separate package")" echo "$(gettext " --allsource Generate a source-only tarball including downloaded sources")" echo "$(gettext " --asroot Allow makepkg to run as root user")" printf "$(gettext " --check Run the check() function in the %s")\n" "$BUILDSCRIPT" @@ -1622,11 +1693,11 @@ ARGLIST=("$@") # Parse Command Line Options. -OPT_SHORT="AcCdefFghiLmop:rRsV" +OPT_SHORT="AcCdefFghiLmop:rRstV" OPT_LONG="allsource,asroot,ignorearch,check,clean,cleancache,nodeps" OPT_LONG+=",noextract,force,forcever:,geninteg,help,holdver" OPT_LONG+=",install,log,nocolor,nobuild,nocheck,pkg:,rmdeps" -OPT_LONG+=",repackage,skipinteg,source,syncdeps,version,config:" +OPT_LONG+=",repackage,skipinteg,source,syncdeps,splitdbg,version,config:" # Pacman Options OPT_LONG+=",noconfirm,noprogressbar" OPT_TEMP="$(parse_options $OPT_SHORT $OPT_LONG "$@" || echo 'PARSE_OPTIONS FAILED')" @@ -1671,6 +1742,7 @@ --skipinteg) SKIPINTEG=1 ;; --source) SOURCEONLY=1 ;; -s|--syncdeps) DEP_BIN=1 ;; + -t|--splitdbg) SPLIT_DBG=1 ;; -h|--help) usage; exit 0 ;; # E_OK -V|--version) version; exit 0 ;; # E_OK @@ -1874,6 +1946,8 @@ devel_check devel_update +[ "$(check_option splitdbg)" = "y" ] || (( SPLIT_DBG )) && add_split_debug_packages + if (( ${#pkgname[@]} > 1 )); then SPLITPKG=1 fi On Wed, Aug 10, 2011 at 9:09 PM, Dan McGee <dpmcgee@gmail.com> wrote:
I rewrite patch from Remy Oudomphen**g Rémy Oudompheng ? Not sure what the ** was for. :) (http://bugs.archlinux.org/task/10975<; http://bugs.archlinux.org/task/10975>) in order to use generic split packaging mechanism. The main idea is that debug packages will be added to pkgname array and for each of it package_<name>-dbg function will be created. So it treated
On Wed, Aug 10, 2011 at 12:04 PM, Vitaliy Gorbunov <vit.gorbunov@gmail.com> wrote: like
usual split packages. Tested on split and non-split packages. All seems to work fine.
Please send inline rather than as an attachment so we can review it, thanks!
-Dan
On Wed, Aug 10, 2011 at 1:25 PM, Vitaliy Gorbunov <vit.gorbunov@gmail.com> wrote:
--- bin/makepkg 2011-08-10 19:37:37.000000000 +0300 +++ local/bin/makepkg4 2011-08-10 19:41:15.000000000 +0300
@@ -44,7 +44,7 @@ srcdir="$startdir/src" pkgdir="$startdir/pkg"
-packaging_options=('strip' 'docs' 'libtool' 'emptydirs' 'zipman' 'purge') +packaging_options=('strip' 'splitdbg' 'docs' 'libtool' 'emptydirs' 'zipman' Let's not try to save chrs here. splitdebug would be nicer. 'purge') other_options=('ccache' 'distcc' 'buildflags' 'makeflags') splitpkg_overrides=('pkgver' 'pkgrel' 'pkgdesc' 'arch' 'license' 'groups' \ 'depends' 'optdepends' 'provides' 'conflicts' 'replaces' \ @@ -74,6 +74,8 @@ CHECKFUNC=0 PKGFUNC=0 SPLITPKG=0 +SPLIT_DBG=0 +SKIP_DEBUG_PACKAGE=0 DBG vs DEBUG- pick one please. And why do we need two different vars here? PKGLIST=()
# Forces the pkgver of the current PKGBUILD. Used by the fakeroot call @@ -902,21 +904,49 @@ done fi
+ local is_splitdbg + [ "$(check_option splitdbg)" = "y" ] || (( SPLIT_DBG )) && is_splitdbg=1 + (( is_splitdbg )) && backup_package_variables "dbg" Too much logic voodoo going on here for me to understand quickly- I'd
Allan, can you review this when you get a chance? Vitaliy, it might also be best for you to take a look at our HACKING document, as well as submitting-patches.txt, as we prefer git formatted patches with a commit message and a signoff (and it ensures you are working against git code rather than outdated 3.5.x code at this point). prefer more conventional conditional blocks.
+ + local dbgdir="$pkgdir-dbg" 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) - /usr/bin/strip $STRIP_SHARED "$binary";; + strip_opt=$STRIP_SHARED;; *application/x-archive*) # Libraries (.a) - /usr/bin/strip $STRIP_STATIC "$binary";; + strip_opt=$STRIP_STATIC;; *application/x-executable*) # Binaries - /usr/bin/strip $STRIP_BINARIES "$binary";; + strip_opt=$STRIP_BINARIES;; + *) continue ;; esac + # Strip symbols + if (( is_splitdbg )); then + # Create directory for the symbol file + mkdir -p "${dbgdir}/usr/lib/debug/${binary%/*}" + if readelf -S $binary | grep "\.gnu_debuglink" &>/dev/null; This is very Linux specific. Not sure what we think about this.
then + # An already stripped binary is a hard link + find ${STRIP_DIRS[@]} -samefile "$binary" 2>/dev/null | while read hardlink ; do + # Is the symbol file for the hardlink in the right dir ? + [ -e "${dbgdir}/usr/lib/debug/${hardlink#/}.debug" ] && \ + [ "${hardlink%/*}" != "${binary%/*}" ] && \ + ln "${dbgdir}/usr/lib/debug/${hardlink#/}.debug" "${dbgdir}/usr/lib/debug/${binary%/*}" && break && madness- not a fan.
+ done + else + # Strip the file + /usr/bin/objcopy --only-keep-debug "$binary" "${dbgdir}/usr/lib/debug/${binary#/}.debug" + /usr/bin/strip $strip_opts "$binary" + /usr/bin/objcopy --add-gnu-debuglink="${dbgdir}/usr/lib/debug/${binary#/}.debug" "$binary" + chmod 644 "${dbgdir}/usr/lib/debug/${binary#/}.debug" + fi + else + /usr/bin/strip $strip_opts "$binary" + fi done fi
@@ -1008,6 +1038,12 @@ }
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. + return + fi + if [[ ! -d $pkgdir ]]; then error "$(gettext "Missing pkg/ directory.")" plain "$(gettext "Aborting...")" @@ -1096,6 +1132,38 @@ fi }
+add_split_debug_packages(){ + [ ${#pkgname[@]} = 1 ] && eval "function package_${pkgname}() { package;}" + + local pkg pkgname_dbg + for pkg in ${pkgname[@]} + do + pkgname_dbg=( ${pkgname_dbg[@]} ${pkg} ${pkg/%/-dbg} ) + eval "function package_${pkg}-dbg() { package_dbg; }" + done + pkgname=( ${pkgname_dbg[@]} ) +} + +package_dbg(){ + check_dbg_files=`find $pkgdir/ -name "*.debug"` + if [ -z "$check_dbg_files" ]; then + SKIP_DEBUG_PACKAGE=1 + return + fi + + restore_package_variables "dbg" + pkgdesc="$pkgdesc (debugging symbols)" + depends=("${pkgname%-dbg}=$pkgver-$pkgrel") + groups=('debug' ${groups[@]/%/-debug}) This seems very magic and arbitrary. + optdepends=() + replaces=() + provides=() + conflicts=() + backup=() Why are we listing all these variables if they are empty; is there a better way? I guess it is because our top-level package variables would be there otherwise, so maybe we have to do this. However you already missed makedepends at quick glance, as well as changelog, and I worry about this duplication. + options=(!strip !splitdbg docs !zipman !purge libtool !emptydirs) + unset install +} + create_srcpackage() { cd "$startdir"
@@ -1423,17 +1491,19 @@ }
backup_package_variables() { - local var + local var suffix + 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 var suffix + 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 @@ -1582,6 +1652,7 @@ echo "$(gettext " -r, --rmdeps Remove installed dependencies after a successful build")" echo "$(gettext " -R, --repackage Repackage contents of the package without rebuilding")" echo "$(gettext " -s, --syncdeps Install missing dependencies with pacman")" + echo "$(gettext " -t, --splitdbg Split debugging symbols into separate package")" echo "$(gettext " --allsource Generate a source-only tarball including downloaded sources")" echo "$(gettext " --asroot Allow makepkg to run as root user")" printf "$(gettext " --check Run the check() function in the %s")\n" "$BUILDSCRIPT" @@ -1622,11 +1693,11 @@ ARGLIST=("$@")
# Parse Command Line Options. -OPT_SHORT="AcCdefFghiLmop:rRsV" +OPT_SHORT="AcCdefFghiLmop:rRstV" OPT_LONG="allsource,asroot,ignorearch,check,clean,cleancache,nodeps" OPT_LONG+=",noextract,force,forcever:,geninteg,help,holdver" OPT_LONG+=",install,log,nocolor,nobuild,nocheck,pkg:,rmdeps" -OPT_LONG+=",repackage,skipinteg,source,syncdeps,version,config:" +OPT_LONG+=",repackage,skipinteg,source,syncdeps,splitdbg,version,config:" # Pacman Options OPT_LONG+=",noconfirm,noprogressbar" OPT_TEMP="$(parse_options $OPT_SHORT $OPT_LONG "$@" || echo 'PARSE_OPTIONS FAILED')" @@ -1671,6 +1742,7 @@ --skipinteg) SKIPINTEG=1 ;; --source) SOURCEONLY=1 ;; -s|--syncdeps) DEP_BIN=1 ;; + -t|--splitdbg) SPLIT_DBG=1 ;;
-h|--help) usage; exit 0 ;; # E_OK -V|--version) version; exit 0 ;; # E_OK @@ -1874,6 +1946,8 @@ devel_check devel_update
+[ "$(check_option splitdbg)" = "y" ] || (( SPLIT_DBG )) && add_split_debug_packages + if (( ${#pkgname[@]} > 1 )); then SPLITPKG=1 fi
On Wed, Aug 10, 2011 at 9:09 PM, Dan McGee <dpmcgee@gmail.com> wrote:
I rewrite patch from Remy Oudomphen**g Rémy Oudompheng ? Not sure what the ** was for. :) (http://bugs.archlinux.org/task/10975<; http://bugs.archlinux.org/task/10975>) in order to use generic split packaging mechanism. The main idea is that debug packages will be added to pkgname array and for each of it package_<name>-dbg function will be created. So it treated
On Wed, Aug 10, 2011 at 12:04 PM, Vitaliy Gorbunov <vit.gorbunov@gmail.com> wrote: like
usual split packages. Tested on split and non-split packages. All seems to work fine.
Please send inline rather than as an attachment so we can review it, thanks!
-Dan
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.
+ pkgdesc="$pkgdesc (debugging symbols)" + depends=("${pkgname%-dbg}=$
+ restore_package_variables "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? Thanks. 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@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 + # Is the symbol file for the hardlink in the right dir ? + if [ -e "${dbgdir}/usr/lib/debug/${hardlink#/}.debug" ]; then + 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" + 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;}" + + local pkg pkgname_with_debug + for pkg in ${pkgname[@]} + do + pkgname_with_debug=( ${pkgname_with_debug[@]} ${pkg} ${pkg/%/-debug} ) + eval "function package_${pkg}-debug() { package_debug; }" + done + pkgname=( ${pkgname_with_debug[@]} ) +} + +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
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.
+ pkgdesc="$pkgdesc (debugging symbols)" + depends=("${pkgname%-dbg}=$
+ restore_package_variables "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@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
--add-gnu-debuglink="/usr/lib/ debug/${binary#/}.debug" "$binary"
objcopy definitely doesn't portably support the --add-gnu-debuglink flag...
Do you mean that .gnu_debiglink section isn't portable or I can add this section manually using objcopy --add-section? Changes: *Use objdump --section-headers instead of readelf -S *style *quoting arrays Signed-off-by: Vitaly Gorbunov <vit.gorbunov@gmail.com> --- scripts/makepkg.sh.in | 90 ++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 82 insertions(+), 8 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index d0a514a..0a7deaa 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='' @@ -1009,21 +1011,53 @@ tidy_install() { done fi + if [[ $(check_option splitdebug) = "y" ]] || (( SPLITDEBUG )); then + local splitdebug=1 + local dbgdir="$pkgdir-debug/usr/lib/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 (( ! splitdebug )); then + strip $strip_opts "$binary" + else + # Create directory for the symbol file + mkdir -p "${dbgdir}/${binary%/*}" + if objdump --section-headers "$binary" | grep "\.gnu_debuglink" &>/dev/null; then + # An already stripped binary is a hard link + find ${STRIP_DIRS[@]} -samefile "$binary" -print0 2>/dev/null | + while read -r hardlink ; do + # Is the symbol file for the hardlink in the right dir ? + if [[ -e "${dbgdir}/${hardlink#/}.debug" ]]; then + if [[ "${hardlink%/*}" != "${binary%/*}" ]]; then + ln "${dbgdir}/${hardlink#/}.debug" "${dbgdir}/${binary%/*}" + break + fi + fi + done + else + # Strip the file + objcopy --only-keep-debug "$binary" "${dbgdir}/${binary#/}.debug" + strip $strip_opts "$binary" + objcopy --add-gnu-debuglink="${dbgdir}/${binary#/}.debug" "$binary" + chmod 644 "${dbgdir}/${binary#/}.debug" + fi + fi done fi @@ -1209,6 +1243,12 @@ check_package() { } create_package() { + if (( SKIPDEBUGPACKAGE )); then + warning "$(gettext "No .debug files for debug package found, skipping...")" + SKIPDEBUGPACKAGE=0 + return + fi + if [[ ! -d $pkgdir ]]; then error "$(gettext "Missing %s directory.")" "pkg/" plain "$(gettext "Aborting...")" @@ -1332,6 +1372,33 @@ create_signature() { fi } +add_split_debug_packages() { + if (( ${#pkgname[@]} == 1 )); then + eval "package_${pkgname}() { package; }" + fi + + local pkg pkgname_with_debug + for pkg in "${pkgname[@]}"; do + pkgname_with_debug=("${pkgname_with_debug[@]}" "${pkg}" "${pkg/%/-debug}") + eval "package_${pkg}-debug() { package_debug; }" + done + pkgname=("${pkgname_with_debug[@]}") +} + +package_debug() { + if [[ ! $(find "$pkgdir/" -name "*.debug") ]]; 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 +1843,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 +1918,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 +1962,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 +2015,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 +2201,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
participants (4)
-
Dan McGee
-
Dave Reisner
-
Vitaliy Gorbunov
-
Vitaly Gorbunov