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

["P. J. McDermott" <pjm at nac dot net>]

On 2012-11-23 13:47, Carlos O'Donell wrote:
> 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 :-)

No problem; in my experience, that's usually a person's first guess. :)

>>> 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?

I think you mean malloc/memusage.sh.  And elf/sotruss.ksh is also
addressed by the BZ #13853 patch.  Searching the source tree, I also see
$"msgind" in nscd/nscd.init.

I'll plan on merging my ldd patch and your BZ #13853 patch, as well as
patching nscd/nscd.init and using printf+gettext instead of
echo+eval_gettext as requested by Dmitry.

Expect to see a revised patch within a day or two. :)

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

If I avoid using eval_gettext, this should be even less of a problem.

>> ---
>>  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).

Indeed.

> 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."

Yeah, it's like an implicit --disable-nls.

>> +  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
>>
[...]
> 
> What kind of testing did you do?

I worked on the eval_gettext code in an interactive Bash shell with some
fairly complex msgid strings (with $VARIABLES, ${VARIABLES}, and
newlines) and variable values with / characters.  Once I got it working,
I implemented it in a shell script to test on dash and compared the 
results of gettext (with LANG=en_US.UTF-8) with those of my functions.
Then I plugged the code directly into ldd.bash.in.

Thanks,
-- 
P. J. McDermott
http://www.pehjota.net/
http://www.pehjota.net/contact.html