[pacman-dev] [PATCH] [makepkg] split debug support
~ Less code repetition ~ Drop -t flag; OPTIONS=(splitdbg) only ~ secure_dir isn't really secure but I couldn't think of a better name Signed-off-by: Arerep Serdna <aepd87@gmail.com> --- Original by Jan M. (funkyou): http://bugs.archlinux.org/task/10975 etc/makepkg.conf.in | 1 + scripts/makepkg.sh.in | 134 ++++++++++++++++++++++++++++++++----------------- 2 files changed, 89 insertions(+), 46 deletions(-) diff --git a/etc/makepkg.conf.in b/etc/makepkg.conf.in index f0d1c44..f744ac3 100644 --- a/etc/makepkg.conf.in +++ b/etc/makepkg.conf.in @@ -62,6 +62,7 @@ BUILDENV=(fakeroot !distcc color !ccache) # A negated option will do the opposite of the comments below. # #-- strip: Strip symbols from binaries/libraries in STRIP_DIRS +#-- splitdbg: Same as above, while keeping the symbols in usr/lib/debug #-- docs: Save doc directories specified by DOC_DIRS #-- libtool: Leave libtool (.la) files in packages #-- emptydirs: Leave empty directories in packages diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 1707245..93548ef 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -43,8 +43,9 @@ BUILDSCRIPT='@BUILDSCRIPT@' startdir="$PWD" srcdir="$startdir/src" pkgdir="$startdir/pkg" +dbgdir="$startdir/dbg" -packaging_options=('strip' 'docs' 'libtool' 'emptydirs' 'zipman' 'purge') +packaging_options=('strip' 'splitdbg' 'docs' 'libtool' 'emptydirs' 'zipman' 'purge') other_options=('ccache' 'distcc' 'makeflags' 'force') splitpkg_overrides=('pkgver' 'pkgrel' 'pkgdesc' 'arch' 'license' 'groups' \ 'depends' 'optdepends' 'provides' 'conflicts' 'replaces' \ @@ -808,6 +809,28 @@ run_package() { run_function "$pkgfunc" } +secure_dir() { + [[ -d $1 ]] || mkdir -p "$1"; chmod a-s "$1" +} + +# strip_flags must be declared local in the inclosing function +pre_strip() { + local binary + while read binary; do + case "$(file -biz "$binary")" in + *application/x-sharedlib*) # Libraries (.so) + strip_flags="$STRIP_SHARED";; + *application/x-archive*) # Libraries (.a) + strip_flags="$STRIP_STATIC";; + *application/x-executable*) # Binaries + strip_flags="$STRIP_BINARIES";; + *) + continue;; + esac + echo "$binary" + done < <(find "${STRIP_DIRS[@]}" -type f -perm -u+w 2>/dev/null) +} + tidy_install() { cd "$pkgdir" msg "$(gettext "Tidying install...")" @@ -865,21 +888,32 @@ tidy_install() { done fi - if [[ $(check_option strip) = y && -n ${STRIP_DIRS[*]} ]]; then - msg2 "$(gettext "Stripping unneeded symbols from binaries and libraries...")" - local binary - find ${STRIP_DIRS[@]} -type f -perm -u+w 2>/dev/null | while read binary ; do - case "$(file -biz "$binary")" in - *compressed-encoding*) # Skip compressed binaries - ;; - *application/x-sharedlib*) # Libraries (.so) - /usr/bin/strip $STRIP_SHARED "$binary";; - *application/x-archive*) # Libraries (.a) - /usr/bin/strip $STRIP_STATIC "$binary";; - *application/x-executable*) # Binaries - /usr/bin/strip $STRIP_BINARIES "$binary";; - esac - done + if [[ -n ${STRIP_DIRS[*]} ]]; then + local strip_flags + if [[ $(check_option splitdbg) = "y" ]]; then + msg2 "$(gettext "Moving debugging symbols from binaries and libraries into separate files...")" + pre_strip | while read binary; do + basename="${binary%%*/}" + dirname="${dirname%/*}" + dbg_file="$basename.debug" + dbg_dest="$dbgdir/usr/lib/debug/$binary.debug" + dbg_destdir="${dbg_dest%/*}" + pushd "$dirname" &>/dev/null + /usr/bin/objcopy --only-keep-debug "$basename" "$dbg_file" + /usr/bin/strip $strip_flags "$basename" + /usr/bin/objcopy --add-gnu-debuglink="$dbg_file" "$basename" + if [[ ! -d $dbg_destdir ]]; then + mkdir -p "$dbg_destdir" + fi + mv "$dbg_file" "$dbg_dest" + popd &>/dev/null + done + elif [[ $(check_option strip) = "y" ]]; then + msg2 "$(gettext "Stripping unneeded symbols from binaries and libraries...")" + pre_strip | while read binary; do + /usr/bin/strip $strip_flags "$binary" + done + fi fi if [[ $(check_option libtool) = "n" ]]; then @@ -1070,12 +1104,23 @@ create_package() { fi } +create_dbgpackage() { + [[ -n $(find "$dbgdir" -type f -name \*.debug -print -quit) && + $(check_option splitdbg) = "y" ]] || return 0 + local pkg="${1:-$pkgname}" + pkgdesc+=" (Debug symbols)" + pkgdir="$dbgdir" + depends=("$pkg") + unset backup build changelog conflicts install optdepends \ + provides replaces source + create_package "$pkg-debug" +} + create_srcpackage() { cd "$startdir" # Get back to our src directory so we can begin with sources. - mkdir -p "$srcdir" - chmod a-s "$srcdir" + secure_dir "$srcdir" cd "$srcdir" if (( ! SKIPINTEG || SOURCEONLY == 2 )); then download_sources @@ -1417,6 +1462,24 @@ restore_package_variables() { done } +process_splitpkg() { + for pkg in ${pkgname[@]}; do + for dir in {pkg,dbg}dir; do + read $dir <<<"${!dir}/$pkg" + secure_dir "${!dir}" + done + backup_package_variables + run_package $pkg + tidy_install + create_package $pkg + create_dbgpackage $pkg + restore_package_variables + for dir in {pkg,dbg}dir; do + read $dir <<<"${!dir%/*}" + done + done +} + # getopt like parser parse_options() { local short_options=$1; shift; @@ -1782,8 +1845,7 @@ else fi if (( GENINTEG )); then - mkdir -p "$srcdir" - chmod a-s "$srcdir" + secure_dir "$srcdir" cd "$srcdir" download_sources generate_checksums @@ -1884,18 +1946,9 @@ if (( INFAKEROOT )); then tidy_install fi create_package + create_dbgpackage else - for pkg in ${pkgname[@]}; do - pkgdir="$pkgdir/$pkg" - mkdir -p "$pkgdir" - chmod a-s "$pkgdir" - backup_package_variables - run_package $pkg - tidy_install - create_package $pkg - restore_package_variables - pkgdir="${pkgdir%/*}" - done + process_splitpkg fi msg "$(gettext "Leaving fakeroot environment.")" @@ -1949,8 +2002,7 @@ fi umask 0022 # get back to our src directory so we can begin with sources -mkdir -p "$srcdir" -chmod a-s "$srcdir" +secure_dir "$srcdir" cd "$srcdir" if (( NOEXTRACT )); then @@ -1989,8 +2041,7 @@ else msg "$(gettext "Removing existing pkg/ directory...")" rm -rf "$pkgdir" fi - mkdir -p "$pkgdir" - chmod a-s "$pkgdir" + secure_dir "$pkgdir" cd "$startdir" # if we are root or if fakeroot is not enabled, then we don't use it @@ -2012,18 +2063,9 @@ else fi fi create_package + create_dbgpackage else - for pkg in ${pkgname[@]}; do - pkgdir="$pkgdir/$pkg" - mkdir -p "$pkgdir" - chmod a-s "$pkgdir" - backup_package_variables - run_package $pkg - tidy_install - create_package $pkg - restore_package_variables - pkgdir="${pkgdir%/*}" - done + process_splitpkg fi else if (( ! REPKG && ( PKGFUNC || SPLITPKG ) )); then -- 1.7.1
ver 2 ~ Regression: STRIP_DIRS should be unquoted to handle /opt/* ~ Bad assignments for $dirname ver 1 ~ Less code repetition ~ Drop -t flag; OPTIONS=(splitdbg) only ~ secure_dir isn't really secure but I couldn't think of a better name Signed-off-by: Arerep Serdna <aepd87 at gmail.com> --- Original by Jan M. (funkyou): http://bugs.archlinux.org/task/10975 Signed-off-by: Arerep Serdna <aepd87@gmail.com> --- etc/makepkg.conf.in | 1 + scripts/makepkg.sh.in | 134 ++++++++++++++++++++++++++++++++----------------- 2 files changed, 89 insertions(+), 46 deletions(-) diff --git a/etc/makepkg.conf.in b/etc/makepkg.conf.in index f0d1c44..f744ac3 100644 --- a/etc/makepkg.conf.in +++ b/etc/makepkg.conf.in @@ -62,6 +62,7 @@ BUILDENV=(fakeroot !distcc color !ccache) # A negated option will do the opposite of the comments below. # #-- strip: Strip symbols from binaries/libraries in STRIP_DIRS +#-- splitdbg: Same as above, while keeping the symbols in usr/lib/debug #-- docs: Save doc directories specified by DOC_DIRS #-- libtool: Leave libtool (.la) files in packages #-- emptydirs: Leave empty directories in packages diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 1707245..bd7babd 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -43,8 +43,9 @@ BUILDSCRIPT='@BUILDSCRIPT@' startdir="$PWD" srcdir="$startdir/src" pkgdir="$startdir/pkg" +dbgdir="$startdir/dbg" -packaging_options=('strip' 'docs' 'libtool' 'emptydirs' 'zipman' 'purge') +packaging_options=('strip' 'splitdbg' 'docs' 'libtool' 'emptydirs' 'zipman' 'purge') other_options=('ccache' 'distcc' 'makeflags' 'force') splitpkg_overrides=('pkgver' 'pkgrel' 'pkgdesc' 'arch' 'license' 'groups' \ 'depends' 'optdepends' 'provides' 'conflicts' 'replaces' \ @@ -808,6 +809,28 @@ run_package() { run_function "$pkgfunc" } +secure_dir() { + [[ -d $1 ]] || mkdir -p "$1"; chmod a-s "$1" +} + +# strip_flags must be declared local in the inclosing function +pre_strip() { + local binary + while read binary; do + case "$(file -biz "$binary")" in + *application/x-sharedlib*) # Libraries (.so) + strip_flags="$STRIP_SHARED";; + *application/x-archive*) # Libraries (.a) + strip_flags="$STRIP_STATIC";; + *application/x-executable*) # Binaries + strip_flags="$STRIP_BINARIES";; + *) + continue;; + esac + echo "$binary" + done < <(find ${STRIP_DIRS[@]} -type f -perm -u+w 2>/dev/null) +} + tidy_install() { cd "$pkgdir" msg "$(gettext "Tidying install...")" @@ -865,21 +888,32 @@ tidy_install() { done fi - if [[ $(check_option strip) = y && -n ${STRIP_DIRS[*]} ]]; then - msg2 "$(gettext "Stripping unneeded symbols from binaries and libraries...")" - local binary - find ${STRIP_DIRS[@]} -type f -perm -u+w 2>/dev/null | while read binary ; do - case "$(file -biz "$binary")" in - *compressed-encoding*) # Skip compressed binaries - ;; - *application/x-sharedlib*) # Libraries (.so) - /usr/bin/strip $STRIP_SHARED "$binary";; - *application/x-archive*) # Libraries (.a) - /usr/bin/strip $STRIP_STATIC "$binary";; - *application/x-executable*) # Binaries - /usr/bin/strip $STRIP_BINARIES "$binary";; - esac - done + if [[ -n ${STRIP_DIRS[*]} ]]; then + local strip_flags + if [[ $(check_option splitdbg) = "y" ]]; then + msg2 "$(gettext "Moving debugging symbols from binaries and libraries into separate files...")" + pre_strip | while read binary; do + basename="${binary%%*/}" + dirname="${binary%/*}" + dbg_file="$basename.debug" + dbg_dest="$dbgdir/usr/lib/debug/$binary.debug" + dbg_destdir="${dbg_dest%/*}" + pushd "$dirname" &>/dev/null + /usr/bin/objcopy --only-keep-debug "$basename" "$dbg_file" + /usr/bin/strip $strip_flags "$basename" + /usr/bin/objcopy --add-gnu-debuglink="$dbg_file" "$basename" + if [[ ! -d $dbg_destdir ]]; then + mkdir -p "$dbg_destdir" + fi + mv "$dbg_file" "$dbg_dest" + popd &>/dev/null + done + elif [[ $(check_option strip) = "y" ]]; then + msg2 "$(gettext "Stripping unneeded symbols from binaries and libraries...")" + pre_strip | while read binary; do + /usr/bin/strip $strip_flags "$binary" + done + fi fi if [[ $(check_option libtool) = "n" ]]; then @@ -1070,12 +1104,23 @@ create_package() { fi } +create_dbgpackage() { + [[ -n $(find "$dbgdir" -type f -name \*.debug -print -quit) && + $(check_option splitdbg) = "y" ]] || return 0 + local pkg="${1:-$pkgname}" + pkgdesc+=" (Debug symbols)" + pkgdir="$dbgdir" + depends=("$pkg") + unset backup build changelog conflicts install optdepends \ + provides replaces source + create_package "$pkg-debug" +} + create_srcpackage() { cd "$startdir" # Get back to our src directory so we can begin with sources. - mkdir -p "$srcdir" - chmod a-s "$srcdir" + secure_dir "$srcdir" cd "$srcdir" if (( ! SKIPINTEG || SOURCEONLY == 2 )); then download_sources @@ -1417,6 +1462,24 @@ restore_package_variables() { done } +process_splitpkg() { + for pkg in ${pkgname[@]}; do + for dir in {pkg,dbg}dir; do + read $dir <<<"${!dir}/$pkg" + secure_dir "${!dir}" + done + backup_package_variables + run_package $pkg + tidy_install + create_package $pkg + create_dbgpackage $pkg + restore_package_variables + for dir in {pkg,dbg}dir; do + read $dir <<<"${!dir%/*}" + done + done +} + # getopt like parser parse_options() { local short_options=$1; shift; @@ -1782,8 +1845,7 @@ else fi if (( GENINTEG )); then - mkdir -p "$srcdir" - chmod a-s "$srcdir" + secure_dir "$srcdir" cd "$srcdir" download_sources generate_checksums @@ -1884,18 +1946,9 @@ if (( INFAKEROOT )); then tidy_install fi create_package + create_dbgpackage else - for pkg in ${pkgname[@]}; do - pkgdir="$pkgdir/$pkg" - mkdir -p "$pkgdir" - chmod a-s "$pkgdir" - backup_package_variables - run_package $pkg - tidy_install - create_package $pkg - restore_package_variables - pkgdir="${pkgdir%/*}" - done + process_splitpkg fi msg "$(gettext "Leaving fakeroot environment.")" @@ -1949,8 +2002,7 @@ fi umask 0022 # get back to our src directory so we can begin with sources -mkdir -p "$srcdir" -chmod a-s "$srcdir" +secure_dir "$srcdir" cd "$srcdir" if (( NOEXTRACT )); then @@ -1989,8 +2041,7 @@ else msg "$(gettext "Removing existing pkg/ directory...")" rm -rf "$pkgdir" fi - mkdir -p "$pkgdir" - chmod a-s "$pkgdir" + secure_dir "$pkgdir" cd "$startdir" # if we are root or if fakeroot is not enabled, then we don't use it @@ -2012,18 +2063,9 @@ else fi fi create_package + create_dbgpackage else - for pkg in ${pkgname[@]}; do - pkgdir="$pkgdir/$pkg" - mkdir -p "$pkgdir" - chmod a-s "$pkgdir" - backup_package_variables - run_package $pkg - tidy_install - create_package $pkg - restore_package_variables - pkgdir="${pkgdir%/*}" - done + process_splitpkg fi else if (( ! REPKG && ( PKGFUNC || SPLITPKG ) )); then -- 1.7.1
General comments: 1) Why do we need a $dbgdir? It is creating a package so just reuse pkgdir. The way I see the layout is: $startdir/pkg/pkgname /pkgname-debug Split packages already use the subdir in $startdir/pkg approach and I see no reason not to extend it for the single package approach. In fact, that is basically the only difference between single and split packages so it would unify that code. As part of that unification, we would get the backing up of variables before entering the package function so we could just override them as needed for the debug package creation. 2) I would much prefer having a flag (--dbgpkg?) over the adding of an option (perhaps with a default setting in makepkg.conf for those that want to always build debug packages). Adding an option in a PKGBUILD "forces" everyone to use it given it overrides makepkg.conf settings. 3) How does this interact with !strip? 4) How does this interact with "makepkg -i" 5) Should this only work with packages that are not arch=any? I.e. in a split package with some binary and some "any" packages, only the binary packages should have debug packages created. At the moment it looks like it will create an empty package. 6) I do not understand what secure_dir is needed for? I do not think that this was part of the original patch. Allan
On Sat, May 22, 2010 at 10:24:39AM +1000, Allan McRae wrote:
General comments:
1) Why do we need a $dbgdir? It is creating a package so just reuse pkgdir. The way I see the layout is:
$startdir/pkg/pkgname /pkgname-debug
Split packages already use the subdir in $startdir/pkg approach and I see no reason not to extend it for the single package approach. In fact, that is basically the only difference between single and split packages so it would unify that code. As part of that unification, we would get the backing up of variables before entering the package function so we could just override them as needed for the debug package creation.
If you're proposing that every package gets treated as split; i.e., pkgname always becomes an array, then I agree. That is the cleanest route. Having pkg/built_sources become pkg/pkgname/built_sources for _all_ packages would mean less complication in makepkg. Until then, the split dbg is needed since the current model can't make assumptions about pkg's first level contents when working with non-split packages. It would make sense to stall this until this gets implemented.
2) I would much prefer having a flag (--dbgpkg?) over the adding of an option (perhaps with a default setting in makepkg.conf for those that want to always build debug packages). Adding an option in a PKGBUILD "forces" everyone to use it given it overrides makepkg.conf settings.
I did away with the flag because it happened to be the only option that had both an entry in OPTIONS and a --flag. It should be easy to place it again. You are saying it should be a boolean in makepkg.conf instead of OPTIONS? I agree with that, but it doesn't make sense to continue allowing strip to override as you explained, though. It's more invasive than splitdbg. And some PKGBUILDS even override settings outside of the scope of options=(), like PKGEXT=.pkg.tar.gz. This is unavoidable with the current model. It would be the first option with both a flag and an entry in makepkg.conf. That means new logic to handle precedence.
3) How does this interact with !strip?
splitdbg always takes precedence over strip This is from my patch, line 893, patched source: if [[ $(check_option splitdbg) = "y" ]]; then ... elif [[ $(check_option strip) = "y" ]]; then ... fi splitdbg must be disabled explicitely; disabling strip alone won't work. Also notice how only _one_ of them may run. Implementing a scheme where OPTIONS=(splitdbg) only makes the debug symbol and OPTIONS=(strip) takes exclusive care of the stripping would be riddled with checks, since objcopy needs to link to binaries and it doesn't make sense for those that haven't been stripped. It'd be slow as molasses.
4) How does this interact with "makepkg -i"
It does not install the -debug packages w/makepkg -i Once 1) gets addressed and -debug packages get added into the pkgname array or a similar metric, this could take place.
5) Should this only work with packages that are not arch=any? I.e. in a split package with some binary and some "any" packages, only the binary packages should have debug packages created. At the moment it looks like it will create an empty package.
Good thinking; I'll add an arch check for the next ver. But you're wrong in assuming it creates an empty package. line 1108: [[ -n $(find "$dbgdir" -type f -name \*.debug -print -quit) && $(check_option splitdbg) = "y" ]] || return 0 If it doesn't find .debug files in startdir/dbg it won't proceed. This is one of the benefits of having a dedicated dir, although the search could be altered to -path pkg/*-debug/, or similar.
6) I do not understand what secure_dir is needed for? I do not think that this was part of the original patch.
The main complaint about the original patch was repetition. Notice how my patch did away with tons of lines with: mkdir dir chmod a-s dir It's more maintainable. It's related to the patch because ./dbg gets created this way.
Allan
-- Andres P
On 22/05/10 11:06, Andres P wrote:
On Sat, May 22, 2010 at 10:24:39AM +1000, Allan McRae wrote:
6) I do not understand what secure_dir is needed for? I do not think that this was part of the original patch.
The main complaint about the original patch was repetition.
Notice how my patch did away with tons of lines with: mkdir dir chmod a-s dir
It's more maintainable. It's related to the patch because ./dbg gets created this way.
what about this: mkdir() { /bin/mkdir $1 chmod a-s $1 } Then we can just use mkdir like normal and everything will get done for us. The FHS guarantees the placement of mkdir so we should be fine using the full path there. I will take a separate patch for that :P Allan
participants (3)
-
Allan McRae
-
Andres P
-
Arerep Serdna