Supporting TAP tests with MSVC and Windows

Each Windows patch should cover all Windows build systems; patches that fix a
problem in the src/tools/msvc build while leaving it broken in the GNU make
build are a bad trend. In this case, I expect you'll need few additional
changes to cover both.

On Thu, Apr 02, 2015 at 06:30:02PM +0900, Michael Paquier wrote:

2) tapcheck does not check if IPC::Run is installed or not. Should we
add a check in the code for that? If yes, should we bypass the test or
fail?

The src/tools/msvc build system officially requires ActivePerl, and I seem to
recall ActivePerl installs IPC::Run by default. A check is optional.

7) TAP tests use sometimes top_builddir directly. I think that's ugly,
and if there are no objections I think that we should change it to use
a dedicated environment variable like TESTTOP or similar.

I sense nothing ugly about a top_builddir reference, but see below.

--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl

open HBA, ">>$tempdir/pgdata/pg_hba.conf";
-print HBA "local replication all trust\n";
+# Windows builds do not support local connections
+if ($Config{osname} ne "MSWin32")
+{
+	print HBA "local replication all trust\n";
+}
print HBA "host replication all 127.0.0.1/32 trust\n";
print HBA "host replication all ::1/128 trust\n";
close HBA;

Test suites that run as part of "make check-world", including all src/bin TAP
suites, _must not_ enable trust authentication on Windows. To do so would
reintroduce CVE-2014-0067. (The standard alternative is to call "pg_regress
--config-auth", which switches a data directory to SSPI authentication.)
Suites not in check-world, though not obligated to follow that rule, will have
a brighter future if they do so anyway.

--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm

@@ -73,9 +74,19 @@ sub tempdir_short
sub standard_initdb
{
my $pgdata = shift;
-	system_or_bail("initdb -D '$pgdata' -A trust -N >/dev/null");
-	system_or_bail("$ENV{top_builddir}/src/test/regress/pg_regress",
-				   '--config-auth', $pgdata);
+
+	if ($Config{osname} eq "MSWin32")
+	{
+		system_or_bail("initdb -D $pgdata -A trust -N > NUL");
+		system_or_bail("$ENV{TESTREGRESS}",
+					   '--config-auth', $pgdata);
+	}
+	else
+	{
+		system_or_bail("initdb -D '$pgdata' -A trust -N >/dev/null");
+		system_or_bail("$ENV{top_builddir}/src/test/regress/pg_regress",
+					   '--config-auth', $pgdata);
+	}
}

Make all build systems set TESTREGRESS, and use it unconditionally.

That's not a complete review, just some highlights.

Thanks,
nm

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Thanks for your input, Noah.

On Fri, Apr 3, 2015 at 3:22 PM, Noah Misch <noah@leadboat.com> wrote:

Each Windows patch should cover all Windows build systems; patches that fix a
problem in the src/tools/msvc build while leaving it broken in the GNU make
build are a bad trend. In this case, I expect you'll need few additional
changes to cover both.

Yes I am planning to do more tests with MinGW as well, once I got the
patch in a more advanced state in May/June. For Cygwin, I am not much
familiar with it yet, but I am sure I'll come up with something.

On Thu, Apr 02, 2015 at 06:30:02PM +0900, Michael Paquier wrote:

2) tapcheck does not check if IPC::Run is installed or not. Should we
add a check in the code for that? If yes, should we bypass the test or
fail?

The src/tools/msvc build system officially requires ActivePerl, and I seem to
recall ActivePerl installs IPC::Run by default. A check is optional.

I recall installing ActivePerl for my own box some time ago. It is
5.16, so now it is outdated, but the package manager does not contain
IPC::Run and I had to install it by hand.

Test suites that run as part of "make check-world", including all src/bin TAP
suites, _must not_ enable trust authentication on Windows. To do so would
reintroduce CVE-2014-0067. (The standard alternative is to call "pg_regress
--config-auth", which switches a data directory to SSPI authentication.)
Suites not in check-world, though not obligated to follow that rule, will have
a brighter future if they do so anyway.

OK. I'll rework this part.

--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm

@@ -73,9 +74,19 @@ sub tempdir_short
sub standard_initdb
{
my $pgdata = shift;
-     system_or_bail("initdb -D '$pgdata' -A trust -N >/dev/null");
-     system_or_bail("$ENV{top_builddir}/src/test/regress/pg_regress",
-                                '--config-auth', $pgdata);
+
+     if ($Config{osname} eq "MSWin32")
+     {
+             system_or_bail("initdb -D $pgdata -A trust -N > NUL");
+             system_or_bail("$ENV{TESTREGRESS}",
+                                        '--config-auth', $pgdata);
+     }
+     else
+     {
+             system_or_bail("initdb -D '$pgdata' -A trust -N >/dev/null");
+             system_or_bail("$ENV{top_builddir}/src/test/regress/pg_regress",
+                                        '--config-auth', $pgdata);
+     }
}

Make all build systems set TESTREGRESS, and use it unconditionally.

Yeah, that would be better.

That's not a complete review, just some highlights.

Thanks again.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

(Thanks Noah for pointing out that the patch did not reach the MLs..)

On Fri, Apr 3, 2015 at 3:40 PM, Michael Paquier wrote:

That's not a complete review, just some highlights.

Thanks again.

Here is a v2 with the following changes:
- Use an environment variable to define where pg_regress is located.
- Use SSPI to access a node in the tests, to secure the test environment.
- Rebase on latest HEAD
- SSL tests are run only if build is configured with openssl

A couple of things to note:
- pg_rewind tests are still disabled, waiting for the outcome of
5519F169.8030406@gmx.net. They will need some tweaks.
- SSL tests can work if an equivalent of cp is available, like
something installed with msysgit... IMO the scripts in src/test/ssl
should be patched to be made more portable (see
/messages/by-id/CAB7nPqQivFxnSjPwkyapa8=HTGm0hfDNvdGcM3=hkK6fPT0+Pg@mail.gmail.com)
- I tested the scripts with MinGW and this patch and got them working.
As prove can fail because of a bad perl interpreter, pointing to
/usr/bin/perl, it is necessary to enforce "PROVE = c:\Perl64\bin\perl
c:\Perl64\bin\prove" or similar. That's not beautiful, but it works,
and the t/ scripts need no further modifications. At least I checked
that.
Regards,
--
Michael

On Fri, Apr 10, 2015 at 11:29 AM, Michael Paquier wrote:

Here is a v2 with the following changes:
- Use an environment variable to define where pg_regress is located.
- Use SSPI to access a node in the tests, to secure the test environment.
- Rebase on latest HEAD
- SSL tests are run only if build is configured with openssl

A couple of things to note:
- pg_rewind tests are still disabled, waiting for the outcome of
5519F169.8030406@gmx.net. They will need some tweaks.
- SSL tests can work if an equivalent of cp is available, like
something installed with msysgit... IMO the scripts in src/test/ssl
should be patched to be made more portable (see
/messages/by-id/CAB7nPqQivFxnSjPwkyapa8=HTGm0hfDNvdGcM3=hkK6fPT0+Pg@mail.gmail.com)
- I tested the scripts with MinGW and this patch and got them working.
As prove can fail because of a bad perl interpreter, pointing to
/usr/bin/perl, it is necessary to enforce "PROVE = c:\Perl64\bin\perl
c:\Perl64\bin\prove" or similar. That's not beautiful, but it works,
and the t/ scripts need no further modifications. At least I checked
that.

And here is v3 with support for pg_rewind tests. One thing that I
noticed with this stuff is that the log redirection fails on Windows
with "cannot access file because it is used by another process",
because of system_or_bail that is used by a set of pg_ctl commands in
RewindTest.pm to stop, start and promote the test servers. I am
attaching a second patch switching those calls from system_or_bail to
IPC's run(), making the log capture method completely consistent
across platforms. In the first patch log redirection is done to NUL to
make the test work even if log output is lost, the split is done for
clarity and those patches should be applied together.

Note as well that this patch uses the following patches fixing
independent issues:
- Replace use of rm in initdb tests by rmtree:
/messages/by-id/CAB7nPqQqzjSHxnCPYCO5vr2dmELt8DqedETqRZmj7TMNqb5Bkg@mail.gmail.com
- Add --debug for pg_rewind remote mode:
/messages/by-id/CAB7nPqSMRFZcfB-b6Un8KvnJKWNLi+qckkXgsy1Fu4dQBif=gw@mail.gmail.com
- Improve sleep processing of pg_rewind, windows being sometimes
susceptible about that as well...
/messages/by-id/CAB7nPqSQfTfge-whbpRD99BEbJOX3Z+Pepwa+TUBxA0fDtuVyg@mail.gmail.com
Regards,
--
Michael

On Sun, Apr 19, 2015 at 10:01 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Note as well that this patch uses the following patches fixing
independent issues:
...

Attached is v4. I added a switch in config.pl to be consistent with
./configure and --enable-tap-tests.
--
Michael

On Mon, Apr 20, 2015 at 9:01 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Sun, Apr 19, 2015 at 10:01 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Note as well that this patch uses the following patches fixing
independent issues:
...

Attached is v4. I added a switch in config.pl to be consistent with
./configure and --enable-tap-tests.

Attached is v5, rebased on HEAD (2c47fe16) after conflicts with
dcae5fac and 54a16df0.
--
Michael

On Fri, Apr 24, 2015 at 11:26 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Mon, Apr 20, 2015 at 9:01 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Sun, Apr 19, 2015 at 10:01 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Note as well that this patch uses the following patches fixing
independent issues:
...

Attached is v4. I added a switch in config.pl to be consistent with
./configure and --enable-tap-tests.

Attached is v5, rebased on HEAD (2c47fe16) after conflicts with
dcae5fac and 54a16df0.

Here is v6, a rebased version on HEAD (79f2b5d). There were some
conflicts with the indentation and some other patches related to
pg_rewind and initdb's tests.
--
Michael

On Tue, May 26, 2015 at 3:39 PM, Michael Paquier wrote:

Here is v6, a rebased version on HEAD (79f2b5d). There were some
conflicts with the indentation and some other patches related to
pg_rewind and initdb's tests.

Attached is v7, rebased on 0b157a0.
--
Michael

On Thu, Jun 25, 2015 at 1:40 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Tue, May 26, 2015 at 3:39 PM, Michael Paquier wrote:

Here is v6, a rebased version on HEAD (79f2b5d). There were some
conflicts with the indentation and some other patches related to
pg_rewind and initdb's tests.

Attached is v7, rebased on 0b157a0.

Attached is v8, rebased on 1ea0620, meaning that it includes all the
fancy improvements in log capture for TAP tests.
--
Michael

On 06/25/2015 07:40 AM, Michael Paquier wrote:

On Tue, May 26, 2015 at 3:39 PM, Michael Paquier wrote:

Here is v6, a rebased version on HEAD (79f2b5d). There were some
conflicts with the indentation and some other patches related to
pg_rewind and initdb's tests.

Attached is v7, rebased on 0b157a0.

Thanks! I fiddled with this a bit more, to centralize more of the
platform-dependent stuff to RewindTest.pm. Also, Windows doesn't have
"cat" and "touch" if you haven't installed MinGW, so I replaced those
calls with built-in perl code.

Can you double-check that the attached still works in your environment?
It works for me now.

Note: I had some trouble installing IPC::Run on my system, with
ActiveState Perl and MSVC. There is no PPM package of that for Windows,
so I had to do "cpan install IPC::Run". That downloaded the MinGW C
compiler and make utility, which took a while. But some of the IPC::Run
regression tests failed, and the installation was aborted. I forced my
way through that "notest install IPC::Run". The next obstacle was that
"vcregress <anything>" no longer worked. It complained about finding
some function in the Install module. Turns out that when it installed
the C compiler and make utility, it also installed a module called
"install" from cpan, which has the same name as the PostgreSQL
Install.pm module. We really should rename our module. I got through
that by manually removing the system install.pm module from the perl
installation's site directory. But after that, it worked great :-).

We need to put some instructions in the docs on how to install IPC::Run
on Windows. I can write up something unless you're eager to.

- Heikki

On 07/24/2015 01:27 PM, Heikki Linnakangas wrote:

On 06/25/2015 07:40 AM, Michael Paquier wrote:

On Tue, May 26, 2015 at 3:39 PM, Michael Paquier wrote:

Here is v6, a rebased version on HEAD (79f2b5d). There were some
conflicts with the indentation and some other patches related to
pg_rewind and initdb's tests.

Attached is v7, rebased on 0b157a0.

Thanks! I fiddled with this a bit more, to centralize more of the
platform-dependent stuff to RewindTest.pm. Also, Windows doesn't have
"cat" and "touch" if you haven't installed MinGW, so I replaced those
calls with built-in perl code.

Can you double-check that the attached still works in your
environment? It works for me now.

Note: I had some trouble installing IPC::Run on my system, with
ActiveState Perl and MSVC. There is no PPM package of that for
Windows, so I had to do "cpan install IPC::Run". That downloaded the
MinGW C compiler and make utility, which took a while. But some of the
IPC::Run regression tests failed, and the installation was aborted. I
forced my way through that "notest install IPC::Run". The next
obstacle was that "vcregress <anything>" no longer worked. It
complained about finding some function in the Install module. Turns
out that when it installed the C compiler and make utility, it also
installed a module called "install" from cpan, which has the same name
as the PostgreSQL Install.pm module. We really should rename our
module. I got through that by manually removing the system install.pm
module from the perl installation's site directory. But after that, it
worked great :-).

We need to put some instructions in the docs on how to install
IPC::Run on Windows. I can write up something unless you're eager to.

AFAIK all you need to do is put Run.pm in the right place. It doesn't
need any building, IIRC, and the current version can be got from
<http://cpansearch.perl.org/src/TODDR/IPC-Run-0.94/lib/IPC/Run.pm&gt;

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Jul 25, 2015 at 3:27 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 07/24/2015 01:27 PM, Heikki Linnakangas wrote:

On 06/25/2015 07:40 AM, Michael Paquier wrote:

On Tue, May 26, 2015 at 3:39 PM, Michael Paquier wrote:

Here is v6, a rebased version on HEAD (79f2b5d). There were some
conflicts with the indentation and some other patches related to
pg_rewind and initdb's tests.

Attached is v7, rebased on 0b157a0.

Thanks! I fiddled with this a bit more, to centralize more of the
platform-dependent stuff to RewindTest.pm. Also, Windows doesn't have "cat"
and "touch" if you haven't installed MinGW, so I replaced those calls with
built-in perl code.

Can you double-check that the attached still works in your environment? It
works for me now.

Note: I had some trouble installing IPC::Run on my system, with
ActiveState Perl and MSVC. There is no PPM package of that for Windows, so I
had to do "cpan install IPC::Run". That downloaded the MinGW C compiler and
make utility, which took a while. But some of the IPC::Run regression tests
failed, and the installation was aborted. I forced my way through that
"notest install IPC::Run". The next obstacle was that "vcregress <anything>"
no longer worked. It complained about finding some function in the Install
module. Turns out that when it installed the C compiler and make utility, it
also installed a module called "install" from cpan, which has the same name
as the PostgreSQL Install.pm module. We really should rename our module. I
got through that by manually removing the system install.pm module from the
perl installation's site directory. But after that, it worked great :-).

We need to put some instructions in the docs on how to install IPC::Run on
Windows. I can write up something unless you're eager to.

AFAIK all you need to do is put Run.pm in the right place. It doesn't need
any building, IIRC, and the current version can be got from
<http://cpansearch.perl.org/src/TODDR/IPC-Run-0.94/lib/IPC/Run.pm&gt;

Yeah, that's exactly what I did myself and put it in a path of
PERL5LIB, and it just worked.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jul 24, 2015 at 08:27:42PM +0300, Heikki Linnakangas wrote:

On 06/25/2015 07:40 AM, Michael Paquier wrote:

Attached is v7, rebased on 0b157a0.

Thanks! I fiddled with this a bit more, to centralize more of the
platform-dependent stuff to RewindTest.pm. Also, Windows doesn't have "cat"
and "touch" if you haven't installed MinGW, so I replaced those calls with
built-in perl code.

My main priority for this patch is to not reintroduce CVE-2014-0067. The
patch is operating in a security minefield:

--- a/src/bin/pg_ctl/t/001_start_stop.pl
+++ b/src/bin/pg_ctl/t/001_start_stop.pl
@@ -15,13 +15,9 @@ command_exit_is([ 'pg_ctl', 'start', '-D', "$tempdir/nonexistent" ],

command_ok([ 'pg_ctl', 'initdb', '-D', "$tempdir/data" ], 'pg_ctl initdb');
command_ok(
-	[   "$ENV{top_builddir}/src/test/regress/pg_regress", '--config-auth',
+	[ $ENV{PG_REGRESS}, '--config-auth',
"$tempdir/data" ],
'configure authentication');
-open CONF, ">>$tempdir/data/postgresql.conf";
-print CONF "listen_addresses = ''\n";
-print CONF "unix_socket_directories = '$tempdir_short'\n";
-close CONF;
command_ok([ 'pg_ctl', 'start', '-D', "$tempdir/data", '-w' ],
'pg_ctl start -w');

Since this series of tests doesn't use standard_initdb, the deleted lines
remain necessary.

@@ -303,7 +297,6 @@ sub run_pg_rewind
}
else
{
-
# Cannot come here normally

Unrelated change.

sub standard_initdb
{
my $pgdata = shift;
system_or_bail('initdb', '-D', "$pgdata", '-A' , 'trust', '-N');
-	system_or_bail("$ENV{top_builddir}/src/test/regress/pg_regress",
-		'--config-auth', $pgdata);
+	system_or_bail($ENV{PG_REGRESS}, '--config-auth', $pgdata);
+
+	my $tempdir_short = tempdir_short;
+
+	open CONF, ">>$pgdata/postgresql.conf";
+	print CONF "\n# Added by TestLib.pm)\n";
+	if ($Config{osname} eq "MSWin32")
+	{
+		print CONF "listen_addresses = '127.0.0.1'\n";
+	}
+	else
+	{
+		print CONF "unix_socket_directories = '$tempdir_short'\n";

This second branch needs listen_addresses=''; it doesn't help to secure the
socket directory if the server still listens on localhost.

+sub configure_hba_for_replication
+{
+	my $pgdata = shift;
+
+	open HBA, ">>$pgdata/pg_hba.conf";
+	print HBA "\n# Allow replication (set up by TestLib.pm)\n";
+	if ($Config{osname} ne "MSWin32")
+	{
+		print HBA "local replication all trust\n";
+	}
+	else
+	{
+		print HBA "host replication all 127.0.0.1/32 sspi include_realm=1 map=regress\n";
+		print HBA "host replication all ::1/128 sspi include_realm=1 map=regress\n";

The second line will make the server fail to start on non-IPv6 systems. You
shouldn't need it if the clients always connect to 127.0.0.1, not "localhost".

+ configure_for_replication("$tempdir/pgdata");

That function name appears nowhere else.

+sub tapcheck
+{
+	InstallTemp();
+
+	my @args = ( "prove", "--verbose", "t/*.pl");
+
+	$ENV{PATH} = "$tmp_installdir/bin;$ENV{PATH}";
+	$ENV{PERL5LIB} = "$topdir/src/test/perl;$ENV{PERL5LIB}";
+	$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
+
+	# Find out all the existing TAP tests by simply looking for t/
+	# in the tree.

This target shall not run the SSL suite, for the same reason check-world shall
not run it. We could offer a distinct vcregress.pl target for TAP suites
excluded from check-world.

nm

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Jul 25, 2015 at 2:27 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 06/25/2015 07:40 AM, Michael Paquier wrote:

On Tue, May 26, 2015 at 3:39 PM, Michael Paquier wrote:

Here is v6, a rebased version on HEAD (79f2b5d). There were some
conflicts with the indentation and some other patches related to
pg_rewind and initdb's tests.

Attached is v7, rebased on 0b157a0.

Thanks! I fiddled with this a bit more, to centralize more of the
platform-dependent stuff to RewindTest.pm. Also, Windows doesn't have "cat"
and "touch" if you haven't installed MinGW, so I replaced those calls with
built-in perl code.

Can you double-check that the attached still works in your environment? It
works for me now.

Sure, I'll double check and update your last version as necessary.

We need to put some instructions in the docs on how to install IPC::Run on
Windows. I can write up something unless you're eager to.

I'll do it. That's fine, but it is going to be closer to a ninja
version of what you did.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Jul 25, 2015 at 4:14 PM, Noah Misch <noah@leadboat.com> wrote:

On Fri, Jul 24, 2015 at 08:27:42PM +0300, Heikki Linnakangas wrote:

On 06/25/2015 07:40 AM, Michael Paquier wrote:

Attached is v7, rebased on 0b157a0.

Thanks! I fiddled with this a bit more, to centralize more of the
platform-dependent stuff to RewindTest.pm. Also, Windows doesn't have "cat"
and "touch" if you haven't installed MinGW, so I replaced those calls with
built-in perl code.

My main priority for this patch is to not reintroduce CVE-2014-0067. The
patch is operating in a security minefield:

Thanks!

--- a/src/bin/pg_ctl/t/001_start_stop.pl
+++ b/src/bin/pg_ctl/t/001_start_stop.pl
@@ -15,13 +15,9 @@ command_exit_is([ 'pg_ctl', 'start', '-D', "$tempdir/nonexistent" ],

command_ok([ 'pg_ctl', 'initdb', '-D', "$tempdir/data" ], 'pg_ctl initdb');
command_ok(
-     [   "$ENV{top_builddir}/src/test/regress/pg_regress", '--config-auth',
+     [ $ENV{PG_REGRESS}, '--config-auth',
"$tempdir/data" ],
'configure authentication');
-open CONF, ">>$tempdir/data/postgresql.conf";
-print CONF "listen_addresses = ''\n";
-print CONF "unix_socket_directories = '$tempdir_short'\n";
-close CONF;
command_ok([ 'pg_ctl', 'start', '-D', "$tempdir/data", '-w' ],
'pg_ctl start -w');

Since this series of tests doesn't use standard_initdb, the deleted lines
remain necessary.

Added with a switch on $config{osname}.

@@ -303,7 +297,6 @@ sub run_pg_rewind
}
else
{
-
# Cannot come here normally

Unrelated change.

Removed.

sub standard_initdb
{
my $pgdata = shift;
system_or_bail('initdb', '-D', "$pgdata", '-A' , 'trust', '-N');
-     system_or_bail("$ENV{top_builddir}/src/test/regress/pg_regress",
-             '--config-auth', $pgdata);
+     system_or_bail($ENV{PG_REGRESS}, '--config-auth', $pgdata);
+
+     my $tempdir_short = tempdir_short;
+
+     open CONF, ">>$pgdata/postgresql.conf";
+     print CONF "\n# Added by TestLib.pm)\n";
+     if ($Config{osname} eq "MSWin32")
+     {
+             print CONF "listen_addresses = '127.0.0.1'\n";
+     }
+     else
+     {
+             print CONF "unix_socket_directories = '$tempdir_short'\n";

This second branch needs listen_addresses=''; it doesn't help to secure the
socket directory if the server still listens on localhost.

Yep. Agreed.

+sub configure_hba_for_replication
+{
+     my $pgdata = shift;
+
+     open HBA, ">>$pgdata/pg_hba.conf";
+     print HBA "\n# Allow replication (set up by TestLib.pm)\n";
+     if ($Config{osname} ne "MSWin32")
+     {
+             print HBA "local replication all trust\n";
+     }
+     else
+     {
+             print HBA "host replication all 127.0.0.1/32 sspi include_realm=1 map=regress\n";
+             print HBA "host replication all ::1/128 sspi include_realm=1 map=regress\n";

The second line will make the server fail to start on non-IPv6 systems. You
shouldn't need it if the clients always connect to 127.0.0.1, not "localhost".

Done.

+ configure_for_replication("$tempdir/pgdata");

That function name appears nowhere else.

This looks like dead code to me. Hence removed.

+sub tapcheck
+{
+     InstallTemp();
+
+     my @args = ( "prove", "--verbose", "t/*.pl");
+
+     $ENV{PATH} = "$tmp_installdir/bin;$ENV{PATH}";
+     $ENV{PERL5LIB} = "$topdir/src/test/perl;$ENV{PERL5LIB}";
+     $ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
+
+     # Find out all the existing TAP tests by simply looking for t/
+     # in the tree.

This target shall not run the SSL suite, for the same reason check-world shall
not run it. We could offer a distinct vcregress.pl target for TAP suites
excluded from check-world.

OK, for the sake of duplicating what GNU does, let's simply ignore it then.

Also I added an equivalent of --enable-tap-tests for this MSVC stuff,
removed afterwards by Heikki. Do we want it or not?

An updated patch is attached.
--
Michael

On Sat, Jul 25, 2015 at 10:54 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

An updated patch is attached.

Attached is v9, that fixes conflicts with 01f6bb4 and recent commits
that added TAP tests in pg_basebackup series.
--
Michael

On 07/29/2015 10:13 AM, Michael Paquier wrote:

On Sat, Jul 25, 2015 at 10:54 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

An updated patch is attached.

Attached is v9, that fixes conflicts with 01f6bb4 and recent commits
that added TAP tests in pg_basebackup series.

Thanks, committed with some more tweaking. Peter just added a
slurp_file() function to TestLib.pm, so I used that to replace the call
to 'cat' instead of my previous snippet. I also fixed the issue in the
pg_basebackup test, that LSN is not necessarily 8 characters long,
slightly differently. Calling pg_xlogfile_name seemed a bit indirect.

I expanded the documentation on how to download and install IPC::Run. I
instructed to add PERL5LIB to buildenv.pl, pointing it to the extracted
IPC-Run-0.94/lib directory. Your phrasing of "putting it into PERL5LIB"
was pretty vague, and in fact the PERL5LIB environment variable was
overwritten in vcregress.pl, so just setting the PERL5LIB variable
wouldn't have worked. I changed vcregress.pl to not do that.

I considered moving the instructions on downloading and installing
IPC::Run in the Requirements section, instead of the Running the
Regression Tests section. But there are additional instructions on
downloading and installing more dependencies in the Building the
Documentation section too. The other dependencies in the Requirements
section affect the built binaries. Well, except for the Diff utility.
We're not totally consistent, but I think it's OK as it is.

Also I added an equivalent of --enable-tap-tests for this MSVC stuff,
removed afterwards by Heikki. Do we want it or not?

On Unix, that option controls whether "make check-world" runs the TAP
tests, but there is no equivalent of "check-world" on Windows. Maybe
there should be, but as long as there isn't, the only thing that the
option did was to stop "vcregress tapcheck" from working, if didn't
enable it in the config file first. That seems silly, so I removed it.
We'll need it if we add an equivalent of "make check-world" to
vcregress, but let's cross that bridge when we get there.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jul 30, 2015 at 1:37 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 07/29/2015 10:13 AM, Michael Paquier wrote:

On Sat, Jul 25, 2015 at 10:54 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

An updated patch is attached.

Attached is v9, that fixes conflicts with 01f6bb4 and recent commits
that added TAP tests in pg_basebackup series.

Thanks, committed with some more tweaking. Peter just added a slurp_file()
function to TestLib.pm, so I used that to replace the call to 'cat' instead
of my previous snippet. I also fixed the issue in the pg_basebackup test,
that LSN is not necessarily 8 characters long, slightly differently. Calling
pg_xlogfile_name seemed a bit indirect.

Thanks!
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

This (commit 13d856e of 2015-07-29) added the following:

--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -242,7 +288,17 @@ sub command_exit_is
 	print("# Running: " . join(" ", @{$cmd}) ."\n");
 	my $h = start $cmd;
 	$h->finish();
-	is($h->result(0), $expected, $test_name);
+
+	# On Windows, the exit status of the process is returned directly as the
+	# process's exit code, while on Unix, it's returned in the high bits
+	# of the exit code (see WEXITSTATUS macro in the standard <sys/wait.h>
+	# header file). IPC::Run's result function always returns exit code >> 8,
+	# assuming the Unix convention, which will always return 0 on Windows as
+	# long as the process was not terminated by an exception. To work around
+	# that, use $h->full_result on Windows instead.
+	my $result = ($Config{osname} eq "MSWin32") ?
+		($h->full_results)[0] : $h->result(0);
+	is($result, $expected, $test_name);
 }

That behavior came up again in the context of a newer IPC::Run test case. I'm
considering changing the IPC::Run behavior such that the above would have been
unnecessary. However, if I do, the above code would want to adapt to handle
pre-change and post-change IPC::Run versions. If you have an opinion on
whether or how IPC::Run should change, I welcome comments on
https://github.com/toddr/IPC-Run/issues/161.