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