Обсуждение: Re: [COMMITTERS] pgsql: Remove -w (--ignore-all-space) option from pg_regress's diff

Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> Remove -w (--ignore-all-space) option from pg_regress's diff calls.

> Looks like this has broken on Windows due to different line endings, 
> which -w hid from us.

Yeah.  I was waiting for brown_bat to report in before bringing this
up on the list, because I don't entirely understand what's happening.
So far it appears that the MSVC builds are fine and the MinGW builds
are not.  How can that be?  Aren't they using the same diff program?
        regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>   
>> Tom Lane wrote:
>>     
>>> Remove -w (--ignore-all-space) option from pg_regress's diff calls.
>>>       
>
>   
>> Looks like this has broken on Windows due to different line endings, 
>> which -w hid from us.
>>     
>
> Yeah.  I was waiting for brown_bat to report in before bringing this
> up on the list, because I don't entirely understand what's happening.
> So far it appears that the MSVC builds are fine and the MinGW builds
> are not.  How can that be?  Aren't they using the same diff program?
>
>             
>   

They might not be using the same CVS programs, though. It appears that 
Windows CVS (which, for example, red_bat uses) translates line endings 
to CRLF, which is why it passed the regression tests, but MinGW CVS does 
not, which I think is is why narwahl and vaquita failed and why dawn_bat 
will probably fail next go round. brown_bat is on Cygwin and we should 
not expect a change there.

cheers

andrew

On Mon, Nov 23, 2009 at 06:58, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> Tom Lane wrote:
>>
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>
>>>
>>> Tom Lane wrote:
>>>
>>>>
>>>> Remove -w (--ignore-all-space) option from pg_regress's diff calls.
>>>>
>>
>>
>>>
>>> Looks like this has broken on Windows due to different line endings,
>>> which -w hid from us.
>>>
>>
>> Yeah.  I was waiting for brown_bat to report in before bringing this
>> up on the list, because I don't entirely understand what's happening.
>> So far it appears that the MSVC builds are fine and the MinGW builds
>> are not.  How can that be?  Aren't they using the same diff program?
>>

In most cases, I think they would not use the same diff program, no.
The MSVC builds would be using the one from the gnuwin32 project, and
the mingw one use the one from mingw. It's probably the same diff
source underneath, but it's most likely built with different options.


> They might not be using the same CVS programs, though. It appears that
> Windows CVS (which, for example, red_bat uses) translates line endings to
> CRLF, which is why it passed the regression tests, but MinGW CVS does not,
> which I think is is why narwahl and vaquita failed and why dawn_bat will
> probably fail next go round. brown_bat is on Cygwin and we should not expect
> a change there.

Yeah, I've seen a lot of weirdness with CVS clients on Windows doing
that differently, so that also seems like a very likely reason.


-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:
> On Mon, Nov 23, 2009 at 06:58, Andrew Dunstan <andrew@dunslane.net> wrote:
>> They might not be using the same CVS programs, though. It appears that
>> Windows CVS (which, for example, red_bat uses) translates line endings to
>> CRLF, which is why it passed the regression tests, but MinGW CVS does not,
>> which I think is is why narwahl and vaquita failed and why dawn_bat will
>> probably fail next go round. brown_bat is on Cygwin and we should not expect
>> a change there.

> Yeah, I've seen a lot of weirdness with CVS clients on Windows doing
> that differently, so that also seems like a very likely reason.

brown_bat is indeed still green, so Andrew's probably fingered the right
component.  I thought for a moment about insisting that Windows
buildfarm members use a non-translating CVS client, but that would still
leave people vulnerable when trying to build from source, if they use a
tarball extractor that converts newlines.

I'm thinking that the most appropriate fix is to have pg_regress
continue to use -w, but only on Windows.  (I notice that ecpg is already
doing it that way, presumably for the same reason of newline
differences.)  A filter such as Andrew mumbled about upthread seems like
more trouble than the problem is worth.  Any actually-interesting
whitespace changes should get caught on other platforms.
        regards, tom lane

On Mon, Nov 23, 2009 at 15:23, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Mon, Nov 23, 2009 at 06:58, Andrew Dunstan <andrew@dunslane.net> wrote:
>>> They might not be using the same CVS programs, though. It appears that
>>> Windows CVS (which, for example, red_bat uses) translates line endings to
>>> CRLF, which is why it passed the regression tests, but MinGW CVS does not,
>>> which I think is is why narwahl and vaquita failed and why dawn_bat will
>>> probably fail next go round. brown_bat is on Cygwin and we should not expect
>>> a change there.
>
>> Yeah, I've seen a lot of weirdness with CVS clients on Windows doing
>> that differently, so that also seems like a very likely reason.
>
> brown_bat is indeed still green, so Andrew's probably fingered the right
> component.  I thought for a moment about insisting that Windows
> buildfarm members use a non-translating CVS client, but that would still
> leave people vulnerable when trying to build from source, if they use a
> tarball extractor that converts newlines.
>
> I'm thinking that the most appropriate fix is to have pg_regress
> continue to use -w, but only on Windows.  (I notice that ecpg is already
> doing it that way, presumably for the same reason of newline
> differences.)  A filter such as Andrew mumbled about upthread seems like
> more trouble than the problem is worth.  Any actually-interesting
> whitespace changes should get caught on other platforms.

Agreed, that seems like the most sensible solution.

-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/

Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>   
>> On Mon, Nov 23, 2009 at 06:58, Andrew Dunstan <andrew@dunslane.net> wrote:
>>     
>>> They might not be using the same CVS programs, though. It appears that
>>> Windows CVS (which, for example, red_bat uses) translates line endings to
>>> CRLF, which is why it passed the regression tests, but MinGW CVS does not,
>>> which I think is is why narwahl and vaquita failed and why dawn_bat will
>>> probably fail next go round. brown_bat is on Cygwin and we should not expect
>>> a change there.
>>>       
>
>   
>> Yeah, I've seen a lot of weirdness with CVS clients on Windows doing
>> that differently, so that also seems like a very likely reason.
>>     
>
> brown_bat is indeed still green, so Andrew's probably fingered the right
> component.  I thought for a moment about insisting that Windows
> buildfarm members use a non-translating CVS client, but that would still
> leave people vulnerable when trying to build from source, if they use a
> tarball extractor that converts newlines.
>
> I'm thinking that the most appropriate fix is to have pg_regress
> continue to use -w, but only on Windows.  (I notice that ecpg is already
> doing it that way, presumably for the same reason of newline
> differences.)  A filter such as Andrew mumbled about upthread seems like
> more trouble than the problem is worth.  Any actually-interesting
> whitespace changes should get caught on other platforms.
>   

Well, the filter could be as simple as something like this in the 
Makefile for the mingw case:
   perl -spi.bak -e 's/(?<!\r)\n$/\r\n/;' expected/*.out   rm expected/*.bak


I can live with either solution.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> I'm thinking that the most appropriate fix is to have pg_regress
>> continue to use -w, but only on Windows.

> Well, the filter could be as simple as something like this in the 
> Makefile for the mingw case:

>     perl -spi.bak -e 's/(?<!\r)\n$/\r\n/;' expected/*.out
>     rm expected/*.bak

I'm not at all thrilled with having the build process intentionally
modify source files.  Quite aside from messing up the file timestamps,
what if this is done on a committer's machine?  If the checked-out
files didn't have CRs, that means his CVS client didn't add them
and probably won't remove them on checkin.
        regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>   
>> Tom Lane wrote:
>>     
>>> I'm thinking that the most appropriate fix is to have pg_regress
>>> continue to use -w, but only on Windows.
>>>       
>
>   
>> Well, the filter could be as simple as something like this in the 
>> Makefile for the mingw case:
>>     
>
>   
>>     perl -spi.bak -e 's/(?<!\r)\n$/\r\n/;' expected/*.out
>>     rm expected/*.bak
>>     
>
> I'm not at all thrilled with having the build process intentionally
> modify source files.  Quite aside from messing up the file timestamps,
> what if this is done on a committer's machine?  If the checked-out
> files didn't have CRs, that means his CVS client didn't add them
> and probably won't remove them on checkin.
>
>             
>   

OK, it's a choice of evils. I'm not that unhappy with what you've done.

cheers

andrew