Thread: Supporting TAP tests with MSVC and Windows

Hi all,
(Adding Peter and Heikki in CC for awareness)

Please find attached a WIP patch to support TAP tests with MSVC. The
tests can be kicked with this command:
vcregress tapcheck

There are a couple of things to note with this patch, and I would like
to have some input for some of those things:
1) I have tweaked some code paths to configure a node correctly, with
for example listen_addresses = 'localhost', or disabling local
settings in pg_hba.conf. Does that sound fine?
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?
3) Temporary installation needs to be done in the top directory,
actually out of src/, because Install.pm scans the contents of src/ to
fetch for example the .samples files and this leads to bad
interactions. Using this location has as well the advantage to install
the binaries for all the tests only once.
4) pg_rewind tests are disabled for now, I am waiting for the outcome
of 5519F169.8030406@gmx.net
5) ssl tests are disabled for now (haven't looked at that yet). They
should be tested if possible. Still need some investigation.
6) the command to access pg_regress is passed using an environment
variable TESTREGRESS.
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.
8) Some commands have needed some tweaks because option parsing on
Windows is... Well... sometimes broken. For example those things do
not work on Windows:
createdb foobar3 -E win1252
initdb PGDATA -X XLOGDIR
Note that modifying those commands does not change the test coverage.
9) @Peter: IPC::Run is not reliable when using h->results, by
switching to (h->full_results)[0] I was able to get results reliably
from a child process.
10) This patch needs to have IPC::Run installed. You need to install
it from CPAN, that's the same deal as for OSX.
11) Windows does not support symlink, so some tests of pg_basebackup
cannot be executed. I can live with this restriction.

I think that's all for now. I'll add this patch to the 2015-06 commit fest.
Regards,
--
Michael

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

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

(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
http://www.postgresql.org/message-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
> http://www.postgresql.org/message-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:
http://www.postgresql.org/message-id/CAB7nPqQqzjSHxnCPYCO5vr2dmELt8DqedETqRZmj7TMNqb5Bkg@mail.gmail.com
- Add --debug for pg_rewind remote mode:
http://www.postgresql.org/message-id/CAB7nPqSMRFZcfB-b6Un8KvnJKWNLi+qckkXgsy1Fu4dQBif=gw@mail.gmail.com
- Improve sleep processing of pg_rewind, windows being sometimes
susceptible about that as well...
http://www.postgresql.org/message-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>


cheers

andrew

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>

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

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

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

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

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

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.

On 2022-08-21 Su 20:40, Noah Misch wrote:
> 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.
>
>


Assuming it changes, we'll have to have a version test here. I don't
think we can have a flag day where we suddenly require IPC::Run's
bleeding edge on Windows. So changing it is a good thing, but it won't
help us much.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mon, Aug 22, 2022 at 09:44:42AM -0400, Andrew Dunstan wrote:
> On 2022-08-21 Su 20:40, Noah Misch wrote:
> > 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.
> 
> Assuming it changes, we'll have to have a version test here. I don't
> think we can have a flag day where we suddenly require IPC::Run's
> bleeding edge on Windows. So changing it is a good thing, but it won't
> help us much.

IPC::Run git now has the change, and we may release soon.  Here's what I plan
to push to make PostgreSQL cope.

On 2022-11-17 Th 01:18, Noah Misch wrote:
> On Mon, Aug 22, 2022 at 09:44:42AM -0400, Andrew Dunstan wrote:
>> On 2022-08-21 Su 20:40, Noah Misch wrote:
>>> 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.
>> Assuming it changes, we'll have to have a version test here. I don't
>> think we can have a flag day where we suddenly require IPC::Run's
>> bleeding edge on Windows. So changing it is a good thing, but it won't
>> help us much.
> IPC::Run git now has the change, and we may release soon.  Here's what I plan
> to push to make PostgreSQL cope.


LGTM


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com