pg_regress breaks on msys

Andrew Dunstan <andrew@dunslane.net> writes:

pg_regress now seems to break on Msys virtual locations:
Example from the buildfarm: http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=snake&amp;dt=2006-07-19%2009:00:00

================== pgsql.4660/src/test/regress/log/initdb.log ===================
The filename, directory name, or volume label syntax is incorrect.

Surely this was tested when the original was prepared?

You can probably blame me instead of Magnus, because I did extensive
fooling with the quoting of the commands issued by pg_regress ... and
no, I don't have msys to test with, that's what the buildfarm is for ;-)

This error message seems pretty thoroughly unhelpful though. Any ideas
what it's unhappy about?

regards, tom lane

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

pg_regress now seems to break on Msys virtual locations:
Example from the buildfarm: http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=snake&amp;dt=2006-07-19%2009:00:00

================== pgsql.4660/src/test/regress/log/initdb.log ===================
The filename, directory name, or volume label syntax is incorrect.

Surely this was tested when the original was prepared?

You can probably blame me instead of Magnus, because I did extensive
fooling with the quoting of the commands issued by pg_regress ... and
no, I don't have msys to test with, that's what the buildfarm is for ;-)

Neither do I right now.

This error message seems pretty thoroughly unhelpful though. Any ideas
what it's unhappy about?

I think we need to change the pg_regress error messages so that it
includes the command string that failed, at least for now.

Then we might know instead of speculating.

It will be either quoting problem or a vitual path problem, I am pretty
sure. The old shell script ran in a bourne-shell-like manner. But
calling system() from a C program will call the Windows command shell,
where the quoting rules are quite different.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

Tom Lane wrote:

This error message seems pretty thoroughly unhelpful though. Any ideas
what it's unhappy about?

I think we need to change the pg_regress error messages so that it
includes the command string that failed, at least for now.

Done, but I bet it doesn't tell us anything we don't know already.

It will be either quoting problem or a vitual path problem, I am pretty
sure. The old shell script ran in a bourne-shell-like manner. But
calling system() from a C program will call the Windows command shell,
where the quoting rules are quite different.

In src/include/port.h we have

/*
* Win32 needs double quotes at the beginning and end of system()
* strings. If not, it gets confused with multiple quoted strings.
* It also requires double-quotes around the executable name and
* any files used for redirection. Other args can use single-quotes.
*
* See the "Notes" section about quotes at:
* http://home.earthlink.net/~rlively/MANUALS/COMMANDS/C/CMD.HTM
*/

The referenced link seems to be dead :-( but AFAICS the pg_regress code
is following the stated rules. Also, how is it getting past the "make
install" step which is quoting things just the same? Puzzling.

regards, tom lane

Tom Lane wrote:

In src/include/port.h we have

/*
* Win32 needs double quotes at the beginning and end of system()
* strings. If not, it gets confused with multiple quoted strings.
* It also requires double-quotes around the executable name and
* any files used for redirection. Other args can use single-quotes.
*
* See the "Notes" section about quotes at:
* http://home.earthlink.net/~rlively/MANUALS/COMMANDS/C/CMD.HTM
*/

The referenced link seems to be dead :-( but AFAICS the pg_regress code
is following the stated rules. Also, how is it getting past the "make
install" step which is quoting things just the same? Puzzling.

I found the description somewhere else and copied it into our header
file:

* From http://www.computerhope.com/cmd.htm:
*
* 1. If all of the following conditions are met, then quote characters
* on the command line are preserved:
*
* - no /S switch
* - exactly two quote characters
* - no special characters between the two quote characters, where special
* is one of: &<>()@^|
* - there are one or more whitespace characters between the the two quote
* characters
* - the string between the two quote characters is the name of an
* executable file.
*
* 2. Otherwise, old behavior is to see if the first character is a quote
* character and if so, strip the leading character and remove the last
* quote character on the command line, preserving any text after the last
* quote character.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:

* From http://www.computerhope.com/cmd.htm:
*
* 1. If all of the following conditions are met, then quote characters
* on the command line are preserved:
*
* - no /S switch
* - exactly two quote characters
* - no special characters between the two quote characters, where special
* is one of: &<>()@^|
* - there are one or more whitespace characters between the the two quote
* characters
* - the string between the two quote characters is the name of an
* executable file.

Hmm, that suggests that our code works *only* if there's white space in
all the paths !? Seems unlikely that this description is fully correct,
or we'd have had problems before.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

* From http://www.computerhope.com/cmd.htm:
*
* 1. If all of the following conditions are met, then quote characters
* on the command line are preserved:
*
* - no /S switch
* - exactly two quote characters
* - no special characters between the two quote characters, where special
* is one of: &<>()@^|
* - there are one or more whitespace characters between the the two quote
* characters
* - the string between the two quote characters is the name of an
* executable file.

Hmm, that suggests that our code works *only* if there's white space in
all the paths !? Seems unlikely that this description is fully correct,
or we'd have had problems before.

It is saying _all_ these have to be true, and we already quote
executables, and the string, so we always have more than two quotes:

* Win32 needs double quotes at the beginning and end of system()
* strings. If not, it gets confused with multiple quoted strings.
* It also requires double-quotes around the executable name and
* any files used for redirection. Other args can use single-quotes.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:

Hmm, that suggests that our code works *only* if there's white space in
all the paths !? Seems unlikely that this description is fully correct,
or we'd have had problems before.

It is saying _all_ these have to be true, and we already quote
executables, and the string, so we always have more than two quotes:

Well, the description is about as clear as mud, because it's not saying
which two quote characters it's talking about. I read it as talking
about the two quote characters around any one word/pathname. If you
think it's talking about the two quote characters we put around the
whole command (the SYSTEMQUOTE dodge), then we're certainly going to
fail the "no special characters" test, because all these commands use
I/O redirection symbols.

regards, tom lane

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Tom Lane wrote:

This error message seems pretty thoroughly unhelpful though. Any ideas
what it's unhappy about?

I think we need to change the pg_regress error messages so that it
includes the command string that failed, at least for now.

Done, but I bet it doesn't tell us anything we don't know already.

Well, we have a result, courtesy of a special run from Stefan:
http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=seahorse&amp;dt=2006-07-19%2017:52:41
has:

Command was: ""C:/msys/1.0/home/pgbuild/pgfarmbuild/HEAD/pgsql.804/src/test/regress/./tmp_check/install/C:/msys/1.0/home/pgbuild/pgfarmbuild/HEAD/inst/bin/initdb" -D "C:/msys/1.0/home/pgbuild/pgfarmbuild/HEAD/pgsql.804/src/test/regress/./tmp_check/data" -L "C:/msys/1.0/home/pgbuild/pgfarmbuild/HEAD/pgsql.804/src/test/regress/./tmp_check/install/C:/msys/1.0/home/pgbuild/pgfarmbuild/HEAD/inst/share/postgresql" --noclean --no-locale >"./log/initdb.log" 2>&1"

The second "C:/msys/1.0/" should not be in the path to initdb.

Not sure how to fix.

cheers

andrew

After looking at the presumably-working uses of system() in initdb and
pg_ctl, I have a theory about the pg_regress problem --- could it be
that Windows system() requires a space between I/O redirection symbols
and pathnames? I see that the pre-existing code consistently puts one,
except in cases like "2>&1":

snprintf(cmd, MAXPGPATH, "%s\"%s\" %s%s < \"%s\" >> \"%s\" 2>&1 &%s",
SYSTEMQUOTE, postgres_path, pgdata_opt, post_opts,
DEVNULL, log_file, SYSTEMQUOTE);

but there's nothing in our docs saying this is necessary ...

regards, tom lane

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Hmm, that suggests that our code works *only* if there's white space in
all the paths !? Seems unlikely that this description is fully correct,
or we'd have had problems before.

It is saying _all_ these have to be true, and we already quote
executables, and the string, so we always have more than two quotes:

Well, the description is about as clear as mud, because it's not saying
which two quote characters it's talking about. I read it as talking
about the two quote characters around any one word/pathname. If you
think it's talking about the two quote characters we put around the
whole command (the SYSTEMQUOTE dodge), then we're certainly going to
fail the "no special characters" test, because all these commands use
I/O redirection symbols.

Right, the top says:

* 1. If all of the following conditions are met, then quote characters
* on the _command_ _line_ are preserved:

It is talking about the entire command string, because this is system()
and there is no distinction between commands and arguments --- it is one
big string.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Andrew Dunstan <andrew@dunslane.net> writes:

Command was: ""C:/msys/1.0/home/pgbuild/pgfarmbuild/HEAD/pgsql.804/src/test/regress/./tmp_check/install/C:/msys/1.0/home/pgbuild/pgfarmbuild/HEAD/inst/bin/initdb" -D "C:/msys/1.0/home/pgbuild/pgfarmbuild/HEAD/pgsql.804/src/test/regress/./tmp_check/data" -L "C:/msys/1.0/home/pgbuild/pgfarmbuild/HEAD/pgsql.804/src/test/regress/./tmp_check/install/C:/msys/1.0/home/pgbuild/pgfarmbuild/HEAD/inst/share/postgresql" --noclean --no-locale >"./log/initdb.log" 2>&1"

The second "C:/msys/1.0/" should not be in the path to initdb.

Ah-hah, so apparently "make install DESTDIR=foo" somehow inserts DESTDIR
after that instead of before it? What we need is a way to determine the
paths that make install used ...

regards, tom lane

Andrew Dunstan <andrew@dunslane.net> writes:

Well, we have a result, courtesy of a special run from Stefan:
http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=seahorse&amp;dt=
2006-07-19%2017:52:41
has:

Command was:
""C:/msys/1.0/home/pgbuild/pgfarmbuild/HEAD/pgsql.804/src/test
/regress/./tmp_check/install/C:/msys/1.0/home/pgbuild/pgfarmbu
ild/HEAD/inst/bin/initdb" -D
"C:/msys/1.0/home/pgbuild/pgfarmbuild/HEAD/pgsql.804/src/test/
regress/./tmp_check/data" -L
"C:/msys/1.0/home/pgbuild/pgfarmbuild/HEAD/pgsql.804/src/test/
regress/./tmp_check/install/C:/msys/1.0/home/pgbuild/pgfarmbui
ld/HEAD/inst/share/postgresql" --noclean --no-locale

"./log/initdb.log" 2>&1"

The second "C:/msys/1.0/" should not be in the path to initdb.

Andrew's on to something, I think. Colons are verboten anywhere in a
filename except position 2, right after a drive letter. The path to
postgresql later in the command will also have problems.

Regards,
Paul Bort

I wrote:

Ah-hah, so apparently "make install DESTDIR=foo" somehow inserts DESTDIR
after that instead of before it? What we need is a way to determine the
paths that make install used ...

AFAICS, the makefiles are just blindly concatenating DESTDIR with bindir
etc, for instance this is how initdb/Makefile installs initdb:

$(INSTALL_PROGRAM) initdb$(X) '$(DESTDIR)$(bindir)/initdb$(X)'

The evidence at hand says that this should produce exactly the same path
string as pg_regress is then using to call initdb. So the question in
my mind now is how come the "make install" step isn't failing. For that
matter, this same path-construction technique was used by the
shellscript... so how come it worked before?

It would be simple enough to make pg_regress strip off a drive letter
from the path strings it receives from the Makefile, but I'm not seeing
a principled way to discover that the "/msys/1.0/" part has to go. How
are the makefiles managing to generate a different value of $(bindir) at
install time than what was passed into pg_regress at build time?

regards, tom lane

Tom Lane wrote:

I wrote:

Ah-hah, so apparently "make install DESTDIR=foo" somehow inserts DESTDIR
after that instead of before it? What we need is a way to determine the
paths that make install used ...

AFAICS, the makefiles are just blindly concatenating DESTDIR with bindir
etc, for instance this is how initdb/Makefile installs initdb:

$(INSTALL_PROGRAM) initdb$(X) '$(DESTDIR)$(bindir)/initdb$(X)'

The evidence at hand says that this should produce exactly the same path
string as pg_regress is then using to call initdb. So the question in
my mind now is how come the "make install" step isn't failing. For that
matter, this same path-construction technique was used by the
shellscript... so how come it worked before?

It would be simple enough to make pg_regress strip off a drive letter
from the path strings it receives from the Makefile, but I'm not seeing
a principled way to discover that the "/msys/1.0/" part has to go. How
are the makefiles managing to generate a different value of $(bindir) at
install time than what was passed into pg_regress at build time?

regards, tom lane

I think we'll need to have the makefile tell us what it thinks the cwd
is, so if it's a virtual path we'll be able to use that.

Compare the install log on the 8.1 branch (from our new buildfarm member
bandicoot) here
http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=bandicoot&amp;dt=2006-07-19%2009%3A52%3A28&amp;stg=check

with what seahorse is showing. Note that the install does not involve
windows paths at all - just Msys virtual paths. But we do need to use
Windows paths for the data files.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

I think we'll need to have the makefile tell us what it thinks the cwd
is, so if it's a virtual path we'll be able to use that.

I don't see where cwd enters into it. The thing I don't understand is
that the value of the make variable $(bindir) is apparently changing.
How can it, when it's been hard-wired into Makefile.global by configure?

regards, tom lane

I wrote:

I don't see where cwd enters into it. The thing I don't understand is
that the value of the make variable $(bindir) is apparently changing.
How can it, when it's been hard-wired into Makefile.global by configure?

After some googling I gather that msys' make has been hacked to
transform paths between actual Windows paths and virtual paths
at what-they-think-are-strategic spots. If this is correct, then
I think our problem is that the method I used to inject the values
of $(bindir) and friends into pg_regress.c ends up supplying actual
Windows paths, where we would much rather it supplied virtual paths.

An alternative method I had considered using was to have pg_regress.c
get the paths by #including pg_config_paths.h. Can anyone say whether
pg_config_paths.h receives real or virtual paths when building under
mingw?

regards, tom lane

Tom Lane wrote:

I wrote:

I don't see where cwd enters into it. The thing I don't understand is
that the value of the make variable $(bindir) is apparently changing.
How can it, when it's been hard-wired into Makefile.global by configure?

After some googling I gather that msys' make has been hacked to
transform paths between actual Windows paths and virtual paths
at what-they-think-are-strategic spots. If this is correct, then
I think our problem is that the method I used to inject the values
of $(bindir) and friends into pg_regress.c ends up supplying actual
Windows paths, where we would much rather it supplied virtual paths.

Unless it also lies on the echoed command line this seems an
unconvincing explanation. The seahorse log says:

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Winline -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -I../../../src/include -I./src/include/port/win32 -DEXEC_BACKEND -I/c/tcl/include "-I../../../src/include/port/win32" '-DPGBINDIR="/home/pgbuild/pgfarmbuild/HEAD/inst/bin"' '-DLIBDIR="/home/pgbuild/pgfarmbuild/HEAD/inst/lib"' '-DPGSHAREDIR="/home/pgbuild/pgfarmbuild/HEAD/inst/share/postgresql"' '-DHOST_TUPLE="i686-pc-mingw32"' '-DMAKEPROG="make"' '-DSHELLPROG="/bin/sh.exe"' -c -o pg_regress.o pg_regress.c

If those -D values are not what it gets then that would be quite evil.

We used to pass these values almost same way when we first did initdb in
C, and I don't recall any such problems. We had:

override CPPFLAGS := -DPGBINDIR=\"$(*bindir*)\" -DPGDATADIR=\"$(*datadir*)\" -DFRONTEND -I$(*libpq_srcdir*) $(*CPPFLAGS*)

There is also this warning, by the way:

pg_regress.c:63: warning: 'shellprog' defined but not used

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

Unless it also lies on the echoed command line this seems an
unconvincing explanation. The seahorse log says:

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Winline -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -I../../../src/include -I./src/include/port/win32 -DEXEC_BACKEND -I/c/tcl/include "-I../../../src/include/port/win32" '-DPGBINDIR="/home/pgbuild/pgfarmbuild/HEAD/inst/bin"' '-DLIBDIR="/home/pgbuild/pgfarmbuild/HEAD/inst/lib"' '-DPGSHAREDIR="/home/pgbuild/pgfarmbuild/HEAD/inst/share/postgresql"' '-DHOST_TUPLE="i686-pc-mingw32"' '-DMAKEPROG="make"' '-DSHELLPROG="/bin/sh.exe"' -c -o pg_regress.o pg_regress.c

If those -D values are not what it gets then that would be quite evil.

Indeed ... but if those *are* what it gets then how can you explain the
constructed paths?

I just committed a change to extract the paths via pg_config_paths.h.
If that doesn't fix it then I guess the next thing is to put in some
debug printout to show what values are really getting compiled in :-(

We used to pass these values almost same way when we first did initdb in
C, and I don't recall any such problems. We had:

override CPPFLAGS := -DPGBINDIR=\"$(*bindir*)\" -DPGDATADIR=\"$(*datadir*)\" -DFRONTEND -I$(*libpq_srcdir*) $(*CPPFLAGS*)

That seems a bit interesting. What are the stars for? I don't see
anything about a syntax like that in my gmake documentation.

There is also this warning, by the way:
pg_regress.c:63: warning: 'shellprog' defined but not used

Good catch, fix committed.

regards, tom lane

I wrote:

I just committed a change to extract the paths via pg_config_paths.h.
If that doesn't fix it then I guess the next thing is to put in some
debug printout to show what values are really getting compiled in :-(

Seems that *did* fix it, which opens a whole new set of questions about
how much you can trust msys' make. However, the latest seahorse results
show we still have a bug or two:

oidjoins ... ok
type_sanity ... ok
opr_sanity ... ok
test geometry ... server stopped
diff command failed: "diff -w "./expected/geometry.out" "./results/geometry.out" >"./results/geometry.diff""
make: *** [check] Error 2

What I think happened here is that diff reported a difference and
pg_regress misinterpreted the exit status as being a hard failure.
Can someone check on whether it's possible to tell the difference
between these cases with Windows diff ?

regards, tom lane

Tom Lane wrote:

We used to pass these values almost same way when we first did initdb in
C, and I don't recall any such problems. We had:

override CPPFLAGS := -DPGBINDIR=\"$(*bindir*)\" -DPGDATADIR=\"$(*datadir*)\" -DFRONTEND -I$(*libpq_srcdir*) $(*CPPFLAGS*)

That seems a bit interesting. What are the stars for? I don't see
anything about a syntax like that in my gmake documentation.

The stars are from my MUA not handling C&P from formatted text as well
as it should in text mode. It should have read:

override CPPFLAGS := -DPGBINDIR=\"$(bindir)\" -DPGDATADIR=\"$(datadir)\" -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS)

cheers

andrew

I wrote:

What I think happened here is that diff reported a difference and
pg_regress misinterpreted the exit status as being a hard failure.
Can someone check on whether it's possible to tell the difference
between these cases with Windows diff ?

So the latest result shows that the return value from system() is
in fact "1":

type_sanity ... ok
opr_sanity ... ok
test geometry ... diff command failed with status 1: "diff -w "./expected/geometry.out" "./results/geometry.out" >"./results/geometry.diff""
server stopped

What I am now wondering is why win32.h defines WIFEXITED and WEXITSTATUS
the way it does. We have not previously been using those macros to test
the result of system() --- at least not in any exercised code path ---
and what I'm thinking is that they are flat out wrong. At least for
testing system(). Are the results of GetExitCodeProcess() and pclose()
really defined differently?

regards, tom lane

Hi Tom-san.

This is very strange.!!

$ make -s
In file included from preproc.y:6668:
pgc.c: In function `yylex':
pgc.c:1539: warning: label `find_rule' defined but not used
C:/MinGW/include/ctype.h: At top level:
pgc.c:3724: warning: `yy_flex_realloc' defined but not used
initdb.c: In function `locale_date_order':
initdb.c:2163: warning: `%x' yields only last 2 digits of year in some locales
pg_backup_tar.c: In function `_tarAddFile':
pg_backup_tar.c:1052: warning: comparison is always false due to limited range of data type
All of PostgreSQL successfully made. Ready to install.

$ make check
make -C ../../../src/port all
make[1]: Entering directory `/home/hi-saito/postgresql-8.2devel-20060720/src/port'
make[1]: Nothing to be done for `all'.
make[1]: Leaving directory `/home/hi-saito/postgresql-8.2devel-20060720/src/port'
make -C ../../../contrib/spi refint.dll autoinc.dll
make[1]: Entering directory `/home/hi-saito/postgresql-8.2devel-20060720/contrib/spi'
make[1]: `refint.dll' is up to date.
make[1]: `autoinc.dll' is up to date.
make[1]: Leaving directory `/home/hi-saito/postgresql-8.2devel-20060720/contrib/spi'
rm -rf ./testtablespace
mkdir ./testtablespace
./pg_regress --temp-install=./tmp_check --top-builddir=../../.. --temp-port=55432 --schedule=./parallel_schedule
--multibyte=SQL_ASCII --load-language=plpgsql
============== creating temporary installation ==============
============== initializing database system ==============
============== starting postmaster ==============
running on port 55432 with pid 1964
============== creating database "regression" ==============
CREATE DATABASE
ALTER DATABASE
============== installing plpgsql ==============
CREATE LANGUAGE
============== running regression test queries ==============
parallel group (13 tests): text oid varchar char name float4 int2 boolean int8 int4 float8 bit
numeric
boolean ... ok
char ... diff command failed with status 1: "diff -w "./expected/char.out"
"./results/char.out" >"./results/char.diff""
server stopped
make: *** [check] Error 2

However,
$ ls -l results/char.diff
ls: results/char.diff: No such file or directory

Ummmmm
$ diff -w "./expected/char.out" "./results/char.out"
66d65
< | A
71c70
< (5 rows)
---

(4 rows)

79d77
< | A
84c82
< (6 rows)
---

(5 rows)

90a89

| A

92c91
< (1 row)
---

(2 rows)

99a99

| A

101c101
< (2 rows)
---

(3 rows)

$ diff -w "./expected/char.out" "./results/char.out" >"./results/char.diff"

$ ls -l results/char.diff
-rw-r--r-- 1 hi-saito pgsql 204 Jul 20 15:23 results/char.diff

hi-saito@SJ157 ~/postgresql-8.2devel-20060720/src/test/regress
$ cat results/char.diff
66d65
< | A
71c70
< (5 rows)
---

(4 rows)

79d77
< | A
84c82
< (6 rows)
---

(5 rows)

90a89

| A

92c91
< (1 row)
---

(2 rows)

99a99

| A

101c101
< (2 rows)
---

(3 rows)

Futhermore, tracking is required.....

Regards,
Hiroshi Saito

"Hiroshi Saito" <z-saito@guitar.ocn.ne.jp> writes:

This is very strange.!!
boolean ... ok
char ... diff command failed with status 1: "diff -w "./expected/char.out"
"./results/char.out" >"./results/char.diff""
server stopped

Yes, I believe the problem is that our Windows versions of the
WIFEXITED/WEXITSTATUS macros are wrong. pg_regress is trying to verify
that the diff call didn't fail entirely (eg, diff not there or failed
to read one of the files), but the status code diff is returning for
"files not the same" is confusing it.

Can anyone check into what the result status conventions really are on
Windows? I am tempted to change the macros to just swap the bytes,
but I dunno what that will do to their existing usages to check the
result of pclose() or win32_waitpid().

regards, tom lane

From: "Tom Lane"

"Hiroshi Saito" <z-saito@guitar.ocn.ne.jp> writes:

This is very strange.!!
boolean ... ok
char ... diff command failed with status 1: "diff -w "./expected/char.out"
"./results/char.out" >"./results/char.diff""
server stopped

Yes, I believe the problem is that our Windows versions of the
WIFEXITED/WEXITSTATUS macros are wrong. pg_regress is trying to verify
that the diff call didn't fail entirely (eg, diff not there or failed
to read one of the files), but the status code diff is returning for
"files not the same" is confusing it.

Probably No, I also suspected it in the beginning. However, char.diff was not created
by some strange condition. I think that WIFEXITED showed the strange condition.
In the place which I showed above, diff makes char.diff from the Manual operation.
I expect that it is related to a system() call.

I am investigating in some other environments. now...
However, It does not clarify yet..:-(

Regards,
Hiroshi Saito

Tom Lane wrote:

"Hiroshi Saito" <z-saito@guitar.ocn.ne.jp> writes:

This is very strange.!!
boolean ... ok
char ... diff command failed with status 1: "diff -w "./expected/char.out"
"./results/char.out" >"./results/char.diff""
server stopped

Yes, I believe the problem is that our Windows versions of the
WIFEXITED/WEXITSTATUS macros are wrong. pg_regress is trying to verify
that the diff call didn't fail entirely (eg, diff not there or failed
to read one of the files), but the status code diff is returning for
"files not the same" is confusing it.

Can anyone check into what the result status conventions really are on
Windows? I am tempted to change the macros to just swap the bytes,
but I dunno what that will do to their existing usages to check the
result of pclose() or win32_waitpid().

I checked on MinGW and system() just returns the value returned by the
application. There isn't any special two-values-in-one layering like is
done on Unix for wait() and the return value from system(). It seems if
the child dies from a signal, the parent dies too, at least in my C
tests.

I propose the following patch which just makes Win32 behave as returning
a single value from system(), and cleans up pg_regress.c. We don't call
wait() or pipe() on Win32. Though this is clearly a bug fix, I am
hesitant to backpatch it to 8.1 or 8.0 because of no problem reports.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:

*** src/test/regress/pg_regress.c	27 Jul 2006 15:37:19 -0000	1.16
--- src/test/regress/pg_regress.c	28 Jul 2006 19:01:12 -0000
***************
*** 813,823 ****
* work for inspecting the results of system().  For the moment,
* hard-wire the check on Windows.
*/
- #ifndef WIN32
if (!WIFEXITED(r) || WEXITSTATUS(r) > 1)
- #else
- 	if (r != 0 && r != 1)
- #endif
{
fprintf(stderr, _("diff command failed with status %d: %s\n"), r, cmd);
exit_nicely(2);

Um, please also remove the comment just above that.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

*** src/test/regress/pg_regress.c	27 Jul 2006 15:37:19 -0000	1.16
--- src/test/regress/pg_regress.c	28 Jul 2006 19:01:12 -0000
***************
*** 813,823 ****
* work for inspecting the results of system().  For the moment,
* hard-wire the check on Windows.
*/
- #ifndef WIN32
if (!WIFEXITED(r) || WEXITSTATUS(r) > 1)
- #else
- 	if (r != 0 && r != 1)
- #endif
{
fprintf(stderr, _("diff command failed with status %d: %s\n"), r, cmd);
exit_nicely(2);

Um, please also remove the comment just above that.

Done.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:

I checked on MinGW and system() just returns the value returned by the
application. There isn't any special two-values-in-one layering like is
done on Unix for wait() and the return value from system(). It seems if
the child dies from a signal, the parent dies too, at least in my C
tests.

The cases that I think we most need to defend against are

(A) diff program not found

(B) diff fails to read one of the input files

I think your proposal handles case B, because diff should return exit
code 2 which we will detect, but what happens in case A? Please test it.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

I checked on MinGW and system() just returns the value returned by the
application. There isn't any special two-values-in-one layering like is
done on Unix for wait() and the return value from system(). It seems if
the child dies from a signal, the parent dies too, at least in my C
tests.

The cases that I think we most need to defend against are

(A) diff program not found

(B) diff fails to read one of the input files

I think your proposal handles case B, because diff should return exit
code 2 which we will detect, but what happens in case A? Please test it.

It returns 1.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

I checked on MinGW and system() just returns the value returned by the
application. There isn't any special two-values-in-one layering like is
done on Unix for wait() and the return value from system(). It seems if
the child dies from a signal, the parent dies too, at least in my C
tests.

The cases that I think we most need to defend against are

(A) diff program not found

(B) diff fails to read one of the input files

I think your proposal handles case B, because diff should return exit
code 2 which we will detect, but what happens in case A? Please test it.

It returns 1.

In summary, on MinGW, files differ or 'diff' not found, returns 1. If
one of the files to be compared does not exist, it returns 2. And of
course, if the files are the same, it returns zero.

I assume MSVC builds will have problem with the diff call.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

The cases that I think we most need to defend against are
(A) diff program not found

In summary, on MinGW, files differ or 'diff' not found, returns 1. If
one of the files to be compared does not exist, it returns 2. And of
course, if the files are the same, it returns zero.

OK. The problem here is that pg_regress is coded to assume that
zero-length output file represents success. Given the above Windows
behavior that is *clearly* not good enough, because that's probably
exactly what we will see after diff-not-found (if the Windows shell
acts like a Unix shell does and creates the ">" target first).

I'd suggest modifying the logic so that zero-length output file with a
nonzero return from the child be treated as a fatal condition (not just
a difference, but bail out).

regards, tom lane

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

The cases that I think we most need to defend against are
(A) diff program not found

In summary, on MinGW, files differ or 'diff' not found, returns 1. If
one of the files to be compared does not exist, it returns 2. And of
course, if the files are the same, it returns zero.

OK. The problem here is that pg_regress is coded to assume that
zero-length output file represents success. Given the above Windows
behavior that is *clearly* not good enough, because that's probably
exactly what we will see after diff-not-found (if the Windows shell
acts like a Unix shell does and creates the ">" target first).

I'd suggest modifying the logic so that zero-length output file with a
nonzero return from the child be treated as a fatal condition (not just
a difference, but bail out).

I modified pg_regress.c to use just the return code to determine if the
diff worked, but I added in a WIN32-specific test for the file size. I
think that is the cleanest solution. Attached.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:

I modified pg_regress.c to use just the return code to determine if the
diff worked, but I added in a WIN32-specific test for the file size. I
think that is the cleanest solution. Attached.

It really needs a comment, along the lines of

/*
* On Windows we'll get exit status 1 if the diff invocation
* failed; so we need a way to distinguish failure from "files
* are different". Check to make sure that a diff file was
* created and is nonempty.
*/

Also the test ought to account for file_size returning -1 if file's
not there, so

+ #ifdef WIN32
+ 	if (WEXITSTATUS(r) == 1 && file_size(filename) <= 0)
...

regards, tom lane

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

I modified pg_regress.c to use just the return code to determine if the
diff worked, but I added in a WIN32-specific test for the file size. I
think that is the cleanest solution. Attached.

It really needs a comment, along the lines of

/*
* On Windows we'll get exit status 1 if the diff invocation
* failed; so we need a way to distinguish failure from "files
* are different". Check to make sure that a diff file was
* created and is nonempty.
*/

Also the test ought to account for file_size returning -1 if file's
not there, so

+ #ifdef WIN32
+ 	if (WEXITSTATUS(r) == 1 && file_size(filename) <= 0)
...

I had already added a comment this morning, but didn't post the updated
patch. I have made the adjustment for -1.

Patch attached and applied.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +