Re: 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