[PATCH] Regression tests in windows ignore white space

In the following thread I discovered that my new regression tests worked
perfectly on windows, but when they were run on linux they failed.

http://www.postgresql.org/message-id/CAApHDvo_YCiPYGDz07MpX9o6EGg=3mmyJTb0ysPTwoTg3c=Tvw@mail.gmail.com

After looking at pg_regress to see which options it passes to diff I
discovered that it passes -w on windows to ignore ALL white space.

The attached simple patch changes this so that it only ignores carriage
returns. It does this by passing --strip-trailing-cr to diff instead of -w.
This should help us few developers who use windows to get our white space
correct in out expected results so that the tests also pass on non windows
platforms.

Regards

David Rowley

On Thu, Dec 26, 2013 at 3:02 PM, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> In the following thread I discovered that my new regression tests worked
> perfectly on windows, but when they were run on linux they failed.
>
> http://www.postgresql.org/message-id/CAApHDvo_YCiPYGDz07MpX9o6EGg=3mmyJTb0ysPTwoTg3c=Tvw@mail.gmail.com
>
> After looking at pg_regress to see which options it passes to diff I
> discovered that it passes -w on windows to ignore ALL white space.
>
> The attached simple patch changes this so that it only ignores carriage
> returns. It does this by passing --strip-trailing-cr to diff instead of -w.
> This should help us few developers who use windows to get our white space
> correct in out expected results so that the tests also pass on non windows
> platforms.
>

I also faced some similar issue few days back and tried to search a
bit for some simpler option, but couldn't spend too much time.
I think it would be good if we can make it work without -w option.

When I tried with your patch on windows, it results into following:

============== running regression test queries ==============
test tablespace ... diff: unrecognized option `--strip-trailing-cr
'
diff: Try `diff --help' for more information.
diff command failed with status 2: "diff --strip-trailing-cr
"e:/../postgresql/src/test/regress/expected/tablespace.out"
"e:/../postgresql/src/test/regress/results/tablespace.out" >
"e:/../postgresql/src/test/regress/results/tablespace.out.diff"
"

Which version of diff you are using?

Version of diff on my m/c is:
diff - GNU diffutils version 2.7

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sun, Dec 29, 2013 at 2:29 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>wrote:

> On Thu, Dec 26, 2013 at 3:02 PM, David Rowley <dgrowleyml(at)gmail(dot)com>
> wrote:
> > In the following thread I discovered that my new regression tests worked
> > perfectly on windows, but when they were run on linux they failed.
> >
> >
> http://www.postgresql.org/message-id/CAApHDvo_YCiPYGDz07MpX9o6EGg=3mmyJTb0ysPTwoTg3c=Tvw@mail.gmail.com
> >
> > After looking at pg_regress to see which options it passes to diff I
> > discovered that it passes -w on windows to ignore ALL white space.
> >
> > The attached simple patch changes this so that it only ignores carriage
> > returns. It does this by passing --strip-trailing-cr to diff instead of
> -w.
> > This should help us few developers who use windows to get our white space
> > correct in out expected results so that the tests also pass on non
> windows
> > platforms.
> >
>
> I also faced some similar issue few days back and tried to search a
> bit for some simpler option, but couldn't spend too much time.
> I think it would be good if we can make it work without -w option.
>
> When I tried with your patch on windows, it results into following:
>
> ============== running regression test queries ==============
> test tablespace ... diff: unrecognized option
> `--strip-trailing-cr
> '
> diff: Try `diff --help' for more information.
> diff command failed with status 2: "diff --strip-trailing-cr
> "e:/../postgresql/src/test/regress/expected/tablespace.out"
> "e:/../postgresql/src/test/regress/results/tablespace.out" >
> "e:/../postgresql/src/test/regress/results/tablespace.out.diff"
> "
>
> Which version of diff you are using?
>
> Version of diff on my m/c is:
> diff - GNU diffutils version 2.7
>
>
I had a bit of a look around on the git repository for diffutils and I've
found at least part of the commit which introduced --strip-trailing-cr

http://git.savannah.gnu.org/cgit/diffutils.git/commit/?id=eefb9adae1642dcb0e2ac523c79998f466e94e77

Although here I can only see that they've added the command line args and
not the actual code which implements stripping the carriage returns. Going
by that it seems that was added back in 2001, but the version you're using
is a bit older than than, it seems 2.7 is 19 years old!

http://git.savannah.gnu.org/cgit/diffutils.git/refs/tags

I'm on 2.8.7 which is only 4 years old.

I know we don't normally go with bleeding edge, but I wondering if we could
make the minimum supported diff version at least 2.8 (at least for windows)
which was released in 2002.

Regards

David Rowley

> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>

On Sun, Dec 29, 2013 at 2:06 AM, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> On Sun, Dec 29, 2013 at 2:29 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>>
>> When I tried with your patch on windows, it results into following:
>>
>> ============== running regression test queries ==============
>> test tablespace ... diff: unrecognized option
>> `--strip-trailing-cr
>> '
>> Which version of diff you are using?
>>
>> Version of diff on my m/c is:
>> diff - GNU diffutils version 2.7
>>
>
> I had a bit of a look around on the git repository for diffutils and I've
> found at least part of the commit which introduced --strip-trailing-cr
>
> http://git.savannah.gnu.org/cgit/diffutils.git/commit/?id=eefb9adae1642dcb0e2ac523c79998f466e94e77
>
> Although here I can only see that they've added the command line args and
> not the actual code which implements stripping the carriage returns. Going
> by that it seems that was added back in 2001, but the version you're using
> is a bit older than than, it seems 2.7 is 19 years old!

For Windows build, I am using whatever latest Git provides rather than
downloading
individual components which might not be good, but I find it
convenient. The latest
Git (1.8.4) download on windows still provides 2.7, which is the
reason I am on older
version. However I agree that it is better to use latest version.

> http://git.savannah.gnu.org/cgit/diffutils.git/refs/tags
>
> I'm on 2.8.7 which is only 4 years old.
>
> I know we don't normally go with bleeding edge, but I wondering if we could
> make the minimum supported diff version at least 2.8 (at least for windows)
> which was released in 2002.

I have checked that for some of the other components like bison, flex,
ActiveState TCL we specify minimum version required, but not for diff.
http://www.postgresql.org/docs/devel/static/install-windows-full.html

+1, for minimum diff version as 2.8.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Dec 30, 2013 at 6:02 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>wrote:

> On Sun, Dec 29, 2013 at 2:06 AM, David Rowley <dgrowleyml(at)gmail(dot)com>
> wrote:
> > On Sun, Dec 29, 2013 at 2:29 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> > wrote:
> >>
> >> When I tried with your patch on windows, it results into following:
> >>
> >> ============== running regression test queries ==============
> >> test tablespace ... diff: unrecognized option
> >> `--strip-trailing-cr
> >> '
> >> Which version of diff you are using?
> >>
> >> Version of diff on my m/c is:
> >> diff - GNU diffutils version 2.7
> >>
> >
> > I had a bit of a look around on the git repository for diffutils and I've
> > found at least part of the commit which introduced --strip-trailing-cr
> >
> >
> http://git.savannah.gnu.org/cgit/diffutils.git/commit/?id=eefb9adae1642dcb0e2ac523c79998f466e94e77
> >
> > Although here I can only see that they've added the command line args and
> > not the actual code which implements stripping the carriage returns.
> Going
> > by that it seems that was added back in 2001, but the version you're
> using
> > is a bit older than than, it seems 2.7 is 19 years old!
>
> For Windows build, I am using whatever latest Git provides rather than
> downloading
> individual components which might not be good, but I find it
> convenient. The latest
> Git (1.8.4) download on windows still provides 2.7, which is the
> reason I am on older
> version. However I agree that it is better to use latest version.
>

It looks like the diff version I'm using is from msys and msys is what is
in my search path rather than the git\bin path. To be honest I didn't
realise that git for windows came with bison and flex, (at least I see
bison.exe and flex.exe in the git\bin path.. I don't remember me putting
them there)

I don't seem to even have git\bin in my %path% environment variable at all,
so all those tools are being picked up from the msys path. I'd need to
remind myself about the msys install process, but since we're already
saying in the docs that msys should be installed, then would the fix for
this not just be as simple as my patch plus a note in the docs to say to
ensure msys\bin occurs before git\bin in the %path% environment var,
minimum supported diff version is 2.8.

Did you install msys? if so does it have a later version of diff?

Regards

David Rowley

>
> > http://git.savannah.gnu.org/cgit/diffutils.git/refs/tags
> >
> > I'm on 2.8.7 which is only 4 years old.
> >
> > I know we don't normally go with bleeding edge, but I wondering if we
> could
> > make the minimum supported diff version at least 2.8 (at least for
> windows)
> > which was released in 2002.
>
> I have checked that for some of the other components like bison, flex,
> ActiveState TCL we specify minimum version required, but not for diff.
> http://www.postgresql.org/docs/devel/static/install-windows-full.html
>
> +1, for minimum diff version as 2.8.
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>

On Mon, Dec 30, 2013 at 11:08 AM, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> On Mon, Dec 30, 2013 at 6:02 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>> For Windows build, I am using whatever latest Git provides rather than
>> downloading
>> individual components which might not be good, but I find it
>> convenient. The latest
>> Git (1.8.4) download on windows still provides 2.7, which is the
>> reason I am on older
>> version. However I agree that it is better to use latest version.
>
>
> It looks like the diff version I'm using is from msys and msys is what is in
> my search path rather than the git\bin path. To be honest I didn't realise
> that git for windows came with bison and flex, (at least I see bison.exe and
> flex.exe in the git\bin path.. I don't remember me putting them there)

Yes, git for windows came with bison,flex, etc required for PG build.

> I don't seem to even have git\bin in my %path% environment variable at all,
> so all those tools are being picked up from the msys path. I'd need to
> remind myself about the msys install process, but since we're already saying
> in the docs that msys should be installed,
I think it is one of the ways of set up on Windows, not every user
uses from msys.

> then would the fix for this not
> just be as simple as my patch plus a note in the docs to say to ensure
> msys\bin occurs before git\bin in the %path% environment var, minimum
> supported diff version is 2.8.

To be honest, I am also not fully aware of the project policy in such
a matter where we have to mention the minimum version of dependent
executable (diff). I request some senior members to please let us know
whether we can make PG dependent on diff version >= 2.8 (atleast
for windows), as there are some of the options which are not available
in older versions.

Another option could be that if version of diff available is >= 2.8, then
use '--strip-trailing-cr', else use existing option '-w', but not
sure if it will
serve the purpose completely.

> Did you install msys? if so does it have a later version of diff?
No.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com