Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode

[Stefano Lattarini]

On 02/03/2012 02:24 PM, Peter Rosin wrote:
> Stefano Lattarini skrev 2012-02-03 13:32:
>> On 02/03/2012 10:07 AM, Peter Rosin wrote:
>>
>>> --- a/lib/depcomp
>>> +++ b/lib/depcomp
>>> @@ -1,7 +1,7 @@
>>>  #! /bin/sh
>>>  # depcomp - compile a program generating dependencies as side-effects
>>>  
>>> -scriptversion=2012-02-03.07; # UTC
>>> +scriptversion=2012-02-03.08; # UTC
>>>  
>>>  # Copyright (C) 1999, 2000, 2003, 2004, 2005, 2006, 2007, 2009, 2010,
>>>  # 2011, 2012 Free Software Foundation, Inc.
>>> @@ -57,6 +57,12 @@ EOF
>>>      ;;
>>>  esac
>>>  
>>> +# A tabulation character.
>>> +tab='      '
>>> +# A newline character.
>>> +nl='
>>> +'
>>> +
>>>  if test -z "$depmode" || test -z "$source" || test -z "$object"; then
>>>    echo "depcomp: Variables source, object and depmode must be set" 1>&2
>>>    exit 1
>>> @@ -162,8 +168,7 @@ gcc)
>>>  ## typically no way to rebuild the header).  We avoid this by adding
>>>  ## dummy dependencies for each header file.  Too bad gcc doesn't do
>>>  ## this for us directly.
>>> -  tr ' ' '
>>> -' < "$tmpdepfile" |
>>> +  tr ' ' $nl < "$tmpdepfile" |
>>>
>> Missing quote around "$nl".  The result will be an error from 'tr',
>> and an empty output passed down the pipe (d'oh!).  Several similar
>> instances in the rest of the patch.
> 
> *snip* further evidence of <something>
> 
> Man, what a brain-fart, how embarrassing!
> 
> Thanks for the review, it's highly appropriate this time.
> 
> In my defense, the testsuite didn't catch it either (no new FAILs anyway).
> But in the closing argument, the prosecution brings up that I didn't even
> check a single outcome of the autoconf test "checking dependency style
> of..."  Big sigh!
>
> Regarding changing the 's/.*['$tab' ]//' pattern into "s/.*[$tab ]//"
> I'm a bit reluctant to have * and ? characters inside " quotes as
> that, on occasion, have resulted in bogus filename expansions.  And
> those bugs are not fun to chase.
>
Wait, what?  Are you sure?  I'd consider any shell doing that so severely
broken that it is not worth supporting.

> So, I went with 's/.*['"$tab"' ]//' instead.
>
This is fine though (as it avoids potential error from unescaped '\'
chars into double-quoted string).

> Maybe depmod.tap should be replaced/rewritten with "compilers" that
> simulate the different depmods?  I could tinker with that a bit...
>
Yeah, I had thought about the possibility of such an approach, but was
reluctant to suggest it, since it would entail non-trivial work that
can only offer brittle and unsure results anyway...  Maybe we should
simply make it clear that 'decomp' only offers "best-effort" support
for non-mainstream compilers (i.e., different from GCC >= 4.x and from
recent Sun Studio or MSVC compilers); if problems show up, the user
should just use "./configure --disable-dependency-tracking" (and maybe
send us a patch if he's kind and motivated enough).

> Anyway, let's take this one more round since it is obvious that I can't
> be 100% trusted today and on top of that have been reading through this
> enough to not see any bugs in it even if clearly marked as such...
>
OK.  I'm already preparing a patch to simplify 'depmod.tap' and decrease
the possibility of spurious FAIL or PASS results (will post it this
evening or tomorrow).

> So, ok to push "depcomp: recognize tabs as whitespace in the dashmstdout mode"
> and the below patch to the msvc branch?
> 
ACK.  And ACK for a merge of msvc into master as well.

Thanks,
  Stefano