[pacman-dev] [PATCH] [makepkg] New patch for split debug support, using generic split packaging mechanism
Dan McGee
dpmcgee at gmail.com
Wed Aug 17 18:25:46 EDT 2011
On Wed, Aug 10, 2011 at 1:25 PM, Vitaliy Gorbunov
<vit.gorbunov at gmail.com> wrote:
> --- bin/makepkg 2011-08-10 19:37:37.000000000 +0300
> +++ local/bin/makepkg4 2011-08-10 19:41:15.000000000 +0300
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).
> @@ -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
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 at gmail.com> wrote:
>
>> On Wed, Aug 10, 2011 at 12:04 PM, Vitaliy Gorbunov
>> <vit.gorbunov at 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
>> 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
More information about the pacman-dev
mailing list