[Rpm-maint] [rpm-software-management/rpm] Add flag to use strip -g instead of full strip on DSOs (RhBug:1663264) (#643)

[Mark Wielaard]

On Sat, 2019-03-16 at 22:56 -0700, pavlinamv wrote:
> From 960860164a55553c5277d3a31cbf73e60c1d4a16 Mon Sep 17 00:00:00 2001
> From: Pavlina Moravcova Varekova <pmoravco at redhat.com>
> Date: Sun, 17 Mar 2019 06:47:26 +0100
> Subject: [PATCH] Add flag to use strip -g instead of full strip on DSOs
>  (RhBug:1663264)
> 
> The find-debuginfo.sh flag -g had exactly this meaning. But from
> version rpm-4.13.0-alpha flag -g changes its behavior. It affects
> both libraries and executables.
> 
> For some packages the original behavior was preferred. That is why
> the new find-debuginfo.sh flag --g-libs is created.

I think the patch is correct.

There is just one little issue. That might just be solved with an extra
line of documentation. It isn't immediately clear if the intention is
that -g and --g-libs are used together or not. As mentioned below it
would be good to document the meaning of given only -g, only --g-libs
or -g and --g-libs together.

> ---
>  scripts/find-debuginfo.sh | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/find-debuginfo.sh b/scripts/find-debuginfo.sh
> index 6e3ba2ce0..285ee51db 100755
> --- a/scripts/find-debuginfo.sh
> +++ b/scripts/find-debuginfo.sh
> @@ -4,6 +4,7 @@
>  #
>  # Usage: find-debuginfo.sh [--strict-build-id] [-g] [-r] [-m] [-i] [-n]
>  #			   [--keep-section SECTION] [--remove-section SECTION]
> +#			   [--g-libs]
>  #	 		   [-j N] [--jobs N]
>  #	 		   [-o debugfiles.list]
>  #	 		   [-S debugsourcefiles.list]
> @@ -16,6 +17,7 @@
>  #			   [builddir]
>  #
>  # The -g flag says to use strip -g instead of full strip on DSOs or EXEs.
> +# The --g-libs flag says to use strip -g instead of full strip on DSOs.

I would explicitly say "ONLY on DSOs" to make clear how it differs from
-g. Assuming the user is intended to only use either "full" -g or
"library-only" --g-libs.

> @@ -68,6 +70,9 @@ lib_rpm_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
>  # With -g arg, pass it to strip on libraries or executables.
>  strip_g=false
>  
> +# With --g-libs arg, pass it to strip on libraries.
> +strip_glibs=false
> +
>  # with -r arg, pass --reloc-debug-sections to eu-strip.
>  strip_r=false
>  
> @@ -135,6 +140,9 @@ while [ $# -gt 0 ]; do
>      unique_debug_src_base=$2
>      shift
>      ;;
> +  --g-libs)
> +    strip_glibs=true
> +    ;;
>    -g)
>      strip_g=true
>      ;;

What do we want to do when the user gives both -g and -g-libs together?
Maybe we want to warn (or error) if the user gives both -g and -g-libs. 
Since it isn't completely clear which functionality the user is
requesting.

Alternatively since -g-libs is a subset of "full" -g we might simply
set strip_g=true if just -g is given. That might need a code tweak
below though.

But whatever choice we make, it should be documented.

> @@ -237,6 +245,9 @@ strip_to_debug()
>    application/x-executable*) g=-g ;;
>    application/x-pie-executable*) g=-g ;;
>    esac
> +  $strip_glibs && case "$(file -bi "$2")" in
> +    application/x-sharedlib*) g=-g ;;
> +  esac
>    eu-strip --remove-comment $r $g ${keep_remove_args} -f "$1" "$2" || exit
>    chmod 444 "$1" || exit
>  }

That looks correct.

> @@ -409,8 +420,12 @@ do_file()
>    # libraries. Other executable ELF files (like kernel modules) don't need it.
>    if [ "$include_minidebug" = "true" -a "$strip_g" = "false" ]; then
>      skip_mini=true
> +    if [ "$strip_glibs" = "false" ]; then
> +      case "$(file -bi "$f")" in
> +        application/x-sharedlib*) skip_mini=false ;;
> +      esac
> +    fi
>      case "$(file -bi "$f")" in
> -      application/x-sharedlib*) skip_mini=false ;;
>        application/x-executable*) skip_mini=false ;;
>        application/x-pie-executable*) skip_mini=false ;;
>      esac

This works correctly, if -g and --g-libs are used exclusively.
But when used together it might depend on how we interpret that
situation.

Cheers,

Mark