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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
