Re: [PATCH 2/2] ldd: Don't use Bash-only $"msgid" quoting

["Carlos O'Donell" <carlos at systemhalted dot org>]

On Fri, Nov 23, 2012 at 1:23 PM, P. J. McDermott <pjm@nac.net> wrote:
> On 2012-11-23 11:03, Carlos O'Donell wrote:
>> Paul,
>
> My first name is Patrick, not Paul. :)

Sorry! I saw P, immediately thought Paul :-)

>> Thank you very much for your patch! :-)
>>
>> Unfortunately the script must function correctly without the presence
>> of gettext.sh.
>>
>> Requiring gettext.sh is a non-starter. It would mean glibc (as a
>> package with it's associated tools) would have a runtime dependency on
>> the gettext runtimes (to install gettext.sh), and I am strongly
>> against that. Dependencies are hard things to get right and we must
>> not add them without very good reasons.
>
> Of course.  I'm not sure how I added ". gettext.sh" without realizing
> that it added a new dependency...
>
>> I would consider a patch that checked for gettext.sh and used it if
>> possible to avoid bash-isms, but not outright require it.
>>
>> Hopefully that gives you some guidance and a way forward.
>
> Well here's a patch that makes ldd use gettext.sh if found and otherwise
> define simple gettext and eval_gettext functions.  Though these
> functions should probably be defined more globally for use in other
> scripts in glibc.
>
> Comments/improvements?

I like this patch *much* more.

However, and here is the point at which you can say no, I'd like to
ask you to take it a step further.

As Joseph pointed out we have an existing bug #13853 that is related.

http://sourceware.org/bugzilla/show_bug.cgi?id=13853

In the BZ I point out that we have 3 files, 2 in addition to
ldd.bash.in, namely:
* debug/xtrace.sh
* debug/memusage.sh

Given that you're on the path to fixing this, would you mind fixing
those two also and thus fixing BZ #13853?

At the moment I don't care if all three get copies of your fix below.

> ---
>  elf/ldd.bash.in |   77 ++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 56 insertions(+), 21 deletions(-)
>
> diff --git a/elf/ldd.bash.in b/elf/ldd.bash.in
> index 86f47e6..5fdbe01 100644
> --- a/elf/ldd.bash.in
> +++ b/elf/ldd.bash.in
> @@ -23,8 +23,41 @@
>  # variable LD_TRACE_LOADED_OBJECTS to a non-empty value.
>
>  # We should be able to find the translation right at the beginning.
> -TEXTDOMAIN=libc
> -TEXTDOMAINDIR=@TEXTDOMAINDIR@
> +export TEXTDOMAIN=libc
> +export TEXTDOMAINDIR=@TEXTDOMAINDIR@
> +
> +if (. 2>/dev/null gettext.sh); then
> +  . gettext.sh
> +else
> +  # No internationalization available; just print the original strings.
> +  gettext ()
> +  {
> +    printf '%s' "$1"
> +  }

I'm happy with this, the use of $"" should go away
(http://www.gnu.org/software/gettext/manual/html_node/bash.html).

Going from translated text to untranslated text seems like a good
compromise with a solution that is simply "Install gettext if you want
translations to work."

> +  eval_gettext ()
> +  {
> +    vars=$({
> +      echo "$1" | sed -n '
> +       # First remove ${VARIABLES}.
> +       s/\${[A-Za-z_][A-Za-z0-9_]*}//g;
> +       # Then get $VARIABLES.
> +       s/[^$]*\$\([A-Za-z_][A-Za-z0-9_]*\)[^$]*/\1\n/gp;'
> +      echo "$1" | sed -n '
> +       # First remove $VARIABLES.
> +       s/\$[A-Za-z_][A-Za-z0-9_]*//g;
> +       # Then get ${VARIABLES}.
> +       s/[^$]*\${\([A-Za-z_][A-Za-z0-9_]*\)}[^$]*/\1\n/gp;'
> +      } | sort | uniq)
> +    sed_script=
> +    for var in $vars; do
> +      # Get the value and escape / characters.
> +      value=$(eval echo \"\$$var\" | sed 's|/|\\/|g')
> +      # Build a sed script to perform the variable expansions.
> +      sed_script="${sed_script}s/\$$var/$value/g;s/\${$var}/$value/g;"
> +    done
> +    printf '%s' "$1" | sed "$sed_script"
> +  }
> +fi
>
>  RTLDLIST=@RTLD@
>  warn=
> @@ -35,24 +68,24 @@ while test $# -gt 0; do
>    case "$1" in
>    --vers | --versi | --versio | --version)
>      echo 'ldd @PKGVERSION@@VERSION@'
> -    printf $"Copyright (C) %s Free Software Foundation, Inc.
> +    printf "$(gettext "Copyright (C) %s Free Software Foundation, Inc.
>  This is free software; see the source for copying conditions.  There is NO
>  warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> -" "2012"
> -    printf $"Written by %s and %s.
> -" "Roland McGrath" "Ulrich Drepper"
> +")" "2012"
> +    printf "$(gettext "Written by %s and %s.
> +")" "Roland McGrath" "Ulrich Drepper"
>      exit 0
>      ;;
>    --h | --he | --hel | --help)
> -    echo $"Usage: ldd [OPTION]... FILE...
> +    echo "$(gettext "Usage: ldd [OPTION]... FILE...
>        --help              print this help and exit
>        --version           print version information and exit
>    -d, --data-relocs       process data relocations
>    -r, --function-relocs   process data and function relocations
>    -u, --unused            print unused direct dependencies
>    -v, --verbose           print all information
> -"
> -    printf $"For bug reporting instructions, please see:\\n%s.\\n" \
> +")"
> +    printf "$(gettext "For bug reporting instructions, please see:\\n%s.\\n")" \
>        "@REPORT_BUGS_TO@"
>      exit 0
>      ;;
> @@ -77,15 +110,15 @@ warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>      shift
>      ;;
>    --v | --ve | --ver)
> -    echo >&2 $"ldd: option \`$1' is ambiguous"
> +    echo >&2 "$(eval_gettext "ldd: option \`\$1' is ambiguous")"
>      exit 1
>      ;;
>    --)          # Stop option processing.
>      shift; break
>      ;;
>    -*)
> -    echo >&2 'ldd:' $"unrecognized option" "\`$1'"
> -    echo >&2 $"Try \`ldd --help' for more information."
> +         echo >&2 'ldd:' "$(gettext "unrecognized option")" "\`$1'"
> +    echo >&2 "$(gettext "Try \`ldd --help' for more information.")"
>      exit 1
>      ;;
>    *)
> @@ -117,8 +150,8 @@ try_trace() {
>
>  case $# in
>  0)
> -  echo >&2 'ldd:' $"missing file arguments"
> -  echo >&2 $"Try \`ldd --help' for more information."
> +  echo >&2 'ldd:' "$(gettext "missing file arguments")"
> +  echo >&2 "$(gettext "Try \`ldd --help' for more information.")"
>    exit 1
>    ;;
>  1)
> @@ -140,14 +173,14 @@ for file do
>       ;;
>    esac
>    if test ! -e "$file"; then
> -    echo "ldd: ${file}:" $"No such file or directory" >&2
> +    echo "ldd: ${file}:" "$(gettext "No such file or directory")" >&2
>      result=1
>    elif test ! -f "$file"; then
> -    echo "ldd: ${file}:" $"not regular file" >&2
> +    echo "ldd: ${file}:" "$(gettext "not regular file")" >&2
>      result=1
>    elif test -r "$file"; then
> -    test -x "$file" || echo 'ldd:' $"\
> -warning: you do not have execution permission for" "\`$file'" >&2
> +    test -x "$file" || echo 'ldd:' "$(gettext "\
> +warning: you do not have execution permission for")" "\`$file'" >&2
>      RTLD=
>      ret=1
>      for rtld in ${RTLDLIST}; do
> @@ -175,7 +208,7 @@ warning: you do not have execution permission for" "\`$file'" >&2
>      1)
>        # This can be a non-ELF binary or no binary at all.
>        nonelf "$file" || {
> -       echo $" not a dynamic executable"
> +       echo "$(gettext "       not a dynamic executable")"
>         result=1
>        }
>        ;;
> @@ -183,12 +216,14 @@ warning: you do not have execution permission for" "\`$file'" >&2
>        try_trace "$RTLD" "$file" || result=1
>        ;;
>      *)
> -      echo 'ldd:' ${RTLD} $"exited with unknown exit code" "($ret)" >&2
> +      echo 'ldd:' ${RTLD} "$(gettext "exited with unknown exit code")" \
> +       "($ret)" >&2
>        exit 1
>        ;;
>      esac
>    else
> -    echo 'ldd:' $"error: you do not have read permission for" "\`$file'" >&2
> +    echo 'ldd:' "$(gettext "error: you do not have read permission for")" \
> +      "\`$file'" >&2
>      result=1
>    fi
>  done
> --
> 1.7.10.4

What kind of testing did you do?

Cheers,
Carlos.