r150960 changed ltmain.sh and broke the build

r150960 changed ltmain.sh and broke the build

[NightStrike]

Dave,

You checked in r150960 here:
http://gcc.gnu.org/ml/gcc-cvs/2009-08/msg00642.html

This change affected ltmain.sh:
http://gcc.gnu.org/viewcvs/trunk/ltmain.sh?r1=150960&r2=150959&pathrev=150960

All of those changes to sed now make sed fail miserably on any mingw
host during the build:

libtool: link: /c/buildbot/vista64-mingw32/mingw-x86-x86/build/build/root/i686-w64-mingw32/bin/ar
rc .libs/libssp.a  ssp.o gets-chk.o memcpy-chk.o memmove-chk.o
mempcpy-chk.o memset-chk.o snprintf-chk.o sprintf-chk.o stpcpy-chk.o
strcat-chk.o strcpy-chk.o strncat-chk.o strncpy-chk.o vsnprintf-chk.o
vsprintf-chk.o
libtool: link: /c/buildbot/vista64-mingw32/mingw-x86-x86/build/build/root/i686-w64-mingw32/bin/ranlib
.libs/libssp.a
/bin/sed: -e expression #2, char 22: Invalid content of \{\}

Re: r150960 changed ltmain.sh and broke the build

[Dave Korn]

NightStrike wrote:
> Dave,
> 
> You checked in r150960 here:
> http://gcc.gnu.org/ml/gcc-cvs/2009-08/msg00642.html
> 
> This change affected ltmain.sh:
> http://gcc.gnu.org/viewcvs/trunk/ltmain.sh?r1=150960&r2=150959&pathrev=150960
> 
> All of those changes to sed now make sed fail miserably on any mingw
> host during the build:
> 
> libtool: link: /c/buildbot/vista64-mingw32/mingw-x86-x86/build/build/root/i686-w64-mingw32/bin/ar
> rc .libs/libssp.a  ssp.o gets-chk.o memcpy-chk.o memmove-chk.o
> mempcpy-chk.o memset-chk.o snprintf-chk.o sprintf-chk.o stpcpy-chk.o
> strcat-chk.o strcpy-chk.o strncat-chk.o strncpy-chk.o vsnprintf-chk.o
> vsprintf-chk.o
> libtool: link: /c/buildbot/vista64-mingw32/mingw-x86-x86/build/build/root/i686-w64-mingw32/bin/ranlib
> .libs/libssp.a
> /bin/sed: -e expression #2, char 22: Invalid content of \{\}

  Argh.  Very sorry for the breakage, I'll get straight onto it.  What
versions of the mingw+msys tools are you using?  Can you please send me a copy
of the generated libtool script, and the output you get when re-running the
failing libtool command manually after adding the "--debug" (non-modal) option?

    cheers,
      DaveK

Re: r150960 changed ltmain.sh and broke the build

[Paolo Bonzini]

>    Argh.  Very sorry for the breakage, I'll get straight onto it.  What
> versions of the mingw+msys tools are you using?  Can you please send me a copy
> of the generated libtool script, and the output you get when re-running the
> failing libtool command manually after adding the "--debug" (non-modal) option?

This is the problem:

  removedotparts="s,/\(\./\)\{1\,\},/,g;s,/\.$,/,"

\, should be , (there is another occurrence).

Also please single-quote the commands.

Paolo

Re: r150960 changed ltmain.sh and broke the build

[Paolo Bonzini]

>    Argh.  Very sorry for the breakage, I'll get straight onto it.  What
> versions of the mingw+msys tools are you using?  Can you please send me a copy
> of the generated libtool script, and the output you get when re-running the
> failing libtool command manually after adding the "--debug" (non-modal) option?

This is the problem:

  removedotparts="s,/\(\./\)\{1\,\},/,g;s,/\.$,/,"

\, should be , (there is another occurrence).

Also please single-quote the commands.

Paolo

Re: r150960 changed ltmain.sh and broke the build

[Dave Korn]

Paolo Bonzini wrote:
>>    Argh.  Very sorry for the breakage, I'll get straight onto it.  What
>> versions of the mingw+msys tools are you using?  Can you please send
>> me a copy
>> of the generated libtool script, and the output you get when
>> re-running the
>> failing libtool command manually after adding the "--debug"
>> (non-modal) option?
> 
> This is the problem:
> 
>  removedotparts="s,/\(\./\)\{1\,\},/,g;s,/\.$,/,"
> 
> \, should be , (there is another occurrence).

  Oops.  Looks like there are different versions of SED that have different
requirements, because on cygwin SED 4.1.5, you *have* to quote the comma in
the curly-braces {min,max} repetitions specifier or it is interpreted as a
separator.

> $ sed -e 's,/\(\./\)\{1\,\},/,g;s,/\.$,/,'
> ./foo/bar/./baz
> ./foo/bar/baz
> 
> $ sed -e 's,/\(\./\)\{1,\},/,g;s,/\.$,/,'
> sed: -e expression #1, char 18: unknown option to `s'
> 
> $

  Maybe the best thing would be to change it to use a different separator?
Ralf, have we discovered a new item for the autoconf man page "portable shell"
chapter?

> Also please single-quote the commands.

  Ok, why?

    cheers,
      DaveK

Re: r150960 changed ltmain.sh and broke the build

[Paolo Bonzini]

On 08/26/2009 12:38 PM, Dave Korn wrote:
>    Oops.  Looks like there are different versions of SED that have different
> requirements, because on cygwin SED 4.1.5, you*have*  to quote the comma in
> the curly-braces {min,max} repetitions specifier or it is interpreted as a
> separator.

Right.  I also got bitten by this recently (and I should have known 
better, given my sed maintainer hat).  You should use another separator 
to be portable.

Paolo

Re: r150960 changed ltmain.sh and broke the build

[Dave Korn]

Dave Korn wrote:

>> Also please single-quote the commands.
> 
>   Ok, why?

  BTW should I do that for all four of the patterns?  And what about $dirname
and $basename, and the couple of dozen other locations in ltmain.sh that use
quoted sed scripts?  (You didn't answer my question about "why" yet so I can't
infer anything.)

    cheers,
      DaveK

Re: r150960 changed ltmain.sh and broke the build

[Paolo Bonzini]

On 08/26/2009 12:48 PM, Dave Korn wrote:
> Dave Korn wrote:
>
>>> Also please single-quote the commands.
>>
>>    Ok, why?
>
>    BTW should I do that for all four of the patterns?  And what about $dirname
> and $basename, and the couple of dozen other locations in ltmain.sh that use
> quoted sed scripts?  (You didn't answer my question about "why" yet so I can't
> infer anything.)

I don't like very much backslash sequences in double-quoted variables. 
It's portable, but somewhat unintuitive.  But this is upstream code 
after all, so it's better to leave it alone.  Sorry for the false alarm.

Paolo

Re: r150960 changed ltmain.sh and broke the build

[Dave Korn]

Paolo Bonzini wrote:
> On 08/26/2009 12:48 PM, Dave Korn wrote:
>> Dave Korn wrote:
>>
>>>> Also please single-quote the commands.
>>>
>>>    Ok, why?
>>
>>    BTW should I do that for all four of the patterns?  And what about
>> $dirname
>> and $basename, and the couple of dozen other locations in ltmain.sh
>> that use
>> quoted sed scripts?  (You didn't answer my question about "why" yet so
>> I can't
>> infer anything.)
> 
> I don't like very much backslash sequences in double-quoted variables.
> It's portable, but somewhat unintuitive.  But this is upstream code
> after all, so it's better to leave it alone.  Sorry for the false alarm.

  No problem, thanks for worrying about it.  Are there any particular
recommendations about/against problematic separator characters in the 's'
command?  I was going to use an '@'.

    cheers,
      DaveK

Re: r150960 changed ltmain.sh and broke the build

[Eric Blake]

Dave Korn <dave.korn.cygwin <at> googlemail.com> writes:

>   No problem, thanks for worrying about it.  Are there any particular
> recommendations about/against problematic separator characters in the 's'
> command?  I was going to use an '@'.

The Autoconf manual suggests using 's|||' rather than 's,,,', since ',' and '@'
can both occur in unquoted filenames, but '|' cannot.

-- 
Eric Blake

Re: r150960 changed ltmain.sh and broke the build

[Dave Korn]

Eric Blake wrote:
> Dave Korn <dave.korn.cygwin <at> googlemail.com> writes:
> 
>>   No problem, thanks for worrying about it.  Are there any particular
>> recommendations about/against problematic separator characters in the 's'
>> command?  I was going to use an '@'.
> 
> The Autoconf manual suggests using 's|||' rather than 's,,,', 

  Where?  Not in the section about sed under 'Limitations of Usual Tools' in
the 2.64 info page, as far as I can see.

> since ',' and '@'
> can both occur in unquoted filenames, but '|' cannot.

  I can't grok that.  In what context could there ever be a confusion between
a sed script and a filename?  What about having a file with an "@" in the name
in the current directory would cause sed to do anything different with a -e
script containing an 's' expression using "@" as a separator?  I must be
feeling a bit slow this afternoon.

    cheers,
      DaveK

Re: r150960 changed ltmain.sh and broke the build

[Ralf Wildenhues]

* Dave Korn wrote on Wed, Aug 26, 2009 at 12:38:50PM CEST:
> Paolo Bonzini wrote:
> > $ sed -e 's,/\(\./\)\{1\,\},/,g;s,/\.$,/,'
> > ./foo/bar/./baz
> > ./foo/bar/baz
> > 
> > $ sed -e 's,/\(\./\)\{1,\},/,g;s,/\.$,/,'
> > sed: -e expression #1, char 18: unknown option to `s'
> > 
> > $
> 
>   Maybe the best thing would be to change it to use a different separator?
> Ralf, have we discovered a new item for the autoconf man page "portable shell"
> chapter?

Not really:

     Patterns should not include the separator (unless escaped), even
     as part of a character class.  In conformance with Posix, the Cray
     `sed' rejects `s/[^/]*$//': use `s,[^/]*$,,'.
[...]
     Portable `sed' regular expressions should use `\' only to escape
     characters in the string `$()*.0123456789[\^n{}'.

Sorry for not catching this.

Cheers,
Ralf

Re: r150960 changed ltmain.sh and broke the build

[Dave Korn]

Ralf Wildenhues wrote:
> * Dave Korn wrote on Wed, Aug 26, 2009 at 12:38:50PM CEST:

>> Ralf, have we discovered a new item for the autoconf man page "portable shell"
>> chapter?
> 
> Not really:
> 
>      Patterns should not include the separator (unless escaped), even
>      as part of a character class.  In conformance with Posix, the Cray
>      `sed' rejects `s/[^/]*$//': use `s,[^/]*$,,'.
> [...]
>      Portable `sed' regular expressions should use `\' only to escape
>      characters in the string `$()*.0123456789[\^n{}'.


  Ah, I didn't read those two conditions as applying simultaneously.  Since
the second condition essentially says that you must only ever escape a special
character to make it non-meaningful or a normal character to make it special,
maybe the first condition say something a bit more like ...

>   Patterns should not include the separator (not even escaped, unless
>   you fancy having to use a semantically significant metacharacter for
>   your separator), even as part of a ...

... shouldn't it?

> Sorry for not catching this.

  NP, all fixed now :)

    cheers,
      DaveK

Re: r150960 changed ltmain.sh and broke the build

[Ralf Wildenhues]

[ trimmed Cc:, added autoconf-patches; followups can remove gcc@ ]
[ http://thread.gmane.org/gmane.comp.gcc.devel/108348 ]

* Dave Korn wrote on Wed, Aug 26, 2009 at 08:16:11PM CEST:
> Ralf Wildenhues wrote:
> >      Patterns should not include the separator (unless escaped), even
> >      as part of a character class.  In conformance with Posix, the Cray
> >      `sed' rejects `s/[^/]*$//': use `s,[^/]*$,,'.
> > [...]
> >      Portable `sed' regular expressions should use `\' only to escape
> >      characters in the string `$()*.0123456789[\^n{}'.
> 
>   Ah, I didn't read those two conditions as applying simultaneously.

I don't think they apply simultaneously in the way you interpreted that.
However, in the  s,x\{1\,\},y,  example, the comma is both a separator
and another metacharacter.  Hmm, maybe those sentences don't really
cover this case.

How about

     Patterns should not include the separator (unless escaped), even
     as part of a character class.  In conformance with Posix, the Cray
     `sed' rejects `s/[^/]*$//': use `s,[^/]*$,,'.  Even escaped,
     patterns should not include separators that are also used as
     metacharacters.  For example, GNU sed 3.02 rejects `s,x\{1\,\},,',
     and is used on MinGW.

?

Cheers,
Ralf

Re: r150960 changed ltmain.sh and broke the build

[Paolo Bonzini]

>>       Patterns should not include the separator (unless escaped), even
>>       as part of a character class.  In conformance with Posix, the Cray
>>       `sed' rejects `s/[^/]*$//': use `s,[^/]*$,,'.
>> [...]
>>       Portable `sed' regular expressions should use `\' only to escape
>>       characters in the string `$()*.0123456789[\^n{}'.
>
>
>    Ah, I didn't read those two conditions as applying simultaneously.  Since
> the second condition essentially says that you must only ever escape a special
> character to make it non-meaningful or a normal character to make it special,
> maybe the first condition say something a bit more like ...
>
>>    Patterns should not include the separator (not even escaped, unless
>>    you fancy having to use a semantically significant metacharacter for
>>    your separator), even as part of a ...

No.  The problem is exactly when you use a semantically significant 
metacharacter for your separator.

\*   => unportable, a\* may become either a* or a\*
\>   => unportable, a\> may become either a> or a\>
\$   => unportable, a\$ may become either a$ or a\$
\,   => unportable, part of \{a,b\}
\|   => unportable, a\|b may become either a|b or a\|b and the latter
         may be parsed as alternation
\/   => in theory unportable in practice it would be insane to introduce
         a special sequence \/
\@ \: etc. => in theory unportable, in practice they should be fine.

All of these are forbidden by the wording in Autoconf manual.

One could add / to the list in the second paragraph, and saying 
something like "only / should be used as a separator if you wish to 
escape it, to avoid inadvertent introduction or escaping of regular 
expression operators".

Or even better, it should be left as is.

Paolo

Re: r150960 changed ltmain.sh and broke the build

[Dave Korn]

Paolo Bonzini wrote:

>>>    Patterns should not include the separator (not even escaped, unless
>>>    you fancy having to use a semantically significant metacharacter for
>>>    your separator), even as part of a ...
> 
> No.  The problem is exactly when you use a semantically significant
> metacharacter for your separator.

  The wording "not even escaped, unless you fancy having to ..." was
rhetorical understatement; the idea is that "having to use ..." is not
something that anyone would fancy doing.  I wasn't suggesting a literal
wording, which is why I wrote "something a bit more like".  Sorry for the
confusing linguistics!

    cheers,
      DaveK