Date: Fri, 3 Apr 2015 02:22:30 -0400
From: Noah Misch <noah@leadboat.com>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>,
 peter_e@gmx.net, Heikki Linnakangas <hlinnakangas@vmware.com>
Subject: Re: Supporting TAP tests with MSVC and Windows
Message-ID: <20150403062230.GA1153652@tornado.leadboat.com>
References: 
 <CAB7nPqTQwphkDfZP07w7yBnbFNDhW_JBAMyCFAkarE2VWg8irQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAB7nPqTQwphkDfZP07w7yBnbFNDhW_JBAMyCFAkarE2VWg8irQ@mail.gmail.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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