Handling of REGRESS_OPTS in MSVC for regression tests
Hi all,
As mentioned on this thread, I have been fighting a bit with the
buildfarm when trying to introduce new PGXS options for isolation and
TAP tests:
/messages/by-id/E1gR4D0-0002Yj-Jw@gemulon.postgresql.org
However it happens that we have more issues than the buildfarm failures
to begin with. One of them it related to the way REGRESS_OPTS is
defined and handled across the whole tree for MSVC.
There are in the tree now a couple of paths using top_builddir or
top_srcdir:
$ git grep REGRESS_OPTS | grep top
contrib/dblink/Makefile:REGRESS_OPTS =
--dlpath=$(top_builddir)/src/test/regress
contrib/pg_stat_statements/Makefile:REGRESS_OPTS = --temp-config
$(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
src/test/modules/commit_ts/Makefile:REGRESS_OPTS =
--temp-config=$(top_srcdir)/src/test/modules/commit_ts/commit_ts.conf
src/test/modules/test_rls_hooks/Makefile:REGRESS_OPTS =
--temp-config=$(top_srcdir)/src/test/modules/test_rls_hooks/rls_hooks.conf
Using top_builddir for dblink is for example fine as this needs to point
out to the place where builds of pg_regress are present. However when
it comes to configuration files we should use top_builddir, and ought to
use top_srcdir instead as VPATH would complain about things gone
missing. So the definition that we make out of it is correct. However
there is an issue with MSVC which thinks that REGRESS_OPTS should only
include top_builddir. This is actually fine per-se as all MSVC tests
run with make-installcheck, and not make-check, however I think that we
should allow top_srcdir to be switched on-the-fly as much as
top_builddir as top_srcdir could be used for something else.
Another way worse issue is that MSVC scripts ignore some configuration
values because of the way values of REGRESS_OPTS are parsed. This
causes some sets of regression tests to never run on Windows. For
example, here is what the command generated for pg_stat_statements, and
what happens with it:
c:/pgbuildfarm/pgbuildroot/HEAD/pgsql.build/Release/pg_regress/pg_regress \
--bindir=c:/pgbuildfarm/pgbuildroot/HEAD/pgsql.build/Release/psql \
--dbname=contrib_regression --temp-config pg_stat_statements
[...]
============== running regression test queries ==============
=====================
All 0 tests passed.
=====================
This causes the tests to pass, but to run nothing as test list is eaten
out as an option value for --temp-config. In this case, what happens is
that --temp-config is set to "pg_stat_statements", leaving the test list
empty. The same cannot happen with test_decoding as the buildfarm uses
a custom module to run its tests (still this module could go away with
PGXS options to control isolation tests).
Attached is a patch which makes MSVC scripts more intelligent by being
able to replace top_srcdir as well by topdir when fetching options.
Thanks to that, MSVC is able to run the tests correctly, by building a
proper command. I think that this is a bug that should be back-patched,
because the tests just don't run, and we likely would miss regressions
because of that.
Unfortunately, if I were to commit that directly, the buildfarm would
turn immediately to red for all Windows members, because when checking
module and contrib tests an "installcheck" happens, and
shared_preload_libraries is not set in this case. When working on the
switch to add isolation and TAP test support to PGXS, test_decoding has
been failing because the installed server did not have wal_level =
logical set up, which is one instance of that. The buildfarm code
installing the Postgres instance on which the tests are run should
configure that properly I think, and one tweak that we could use for the
time being is to bypass tests of modules which cannot work yet, so as
the buildfarm keeps being green. This way, this would not be a blocker
for the integration of the new PGXS infra for TAP and isolation. And
those could be enabled back once the buildfarm code is able to set up a
proper configuration. The following modules require some extra
configuration:
- commit_ts
- test_rls_hooks
- pg_stat_statements
- test_decoding (if its Makefile is rewritten)
- snapshot_too_old (if its Makefile is rewritten)
The buildfarm part requires Andrew Dunstan's input, and there is a bit
to sort out for the rest, so input from all is welcome. Please note
that I would prefer if the tests which just cannot work yet are
disabled until the needed buildfarm infrastructure is needed. Another
option could also be to switch contribcheck and modulescheck so as they
use a temporary installation, but that's a can of worms waiting to
explode as MSVC makes more complicated the search for initdb and such
(see the way upgradecheck happens for example), and this makes the tests
waaay longer to run.
Thoughts?
--
Michael
Attachments:
fix-msvc-regress-opts.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 599b521014..f3394905f0 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -619,13 +619,18 @@ sub fetchRegressOpts
$m =~ s{\\\r?\n}{}g;
if ($m =~ /^\s*REGRESS_OPTS\s*\+?=(.*)/m)
{
+ my @split_opts = split(/\s+/, $1);
- # Substitute known Makefile variables, then ignore options that retain
- # an unhandled variable reference. Ignore anything that isn't an
- # option starting with "--".
- @opts = grep { !/\$\(/ && /^--/ }
- map { (my $x = $_) =~ s/\Q$(top_builddir)\E/\"$topdir\"/; $x; }
- split(/\s+/, $1);
+ # Substitute known Makefile variables for each option by
+ # something which is supported in this context.
+ foreach my $opt (@split_opts)
+ {
+ # ignore empty strings
+ next if ($opt =~ /^\s*$/);
+ $opt =~ s#\$\(top_builddir\)#"$topdir"#gs;
+ $opt =~ s#\$\(top_srcdir\)#"$topdir"#gs;
+ push @opts, $opt;
+ }
}
if ($m =~ /^\s*ENCODING\s*=\s*(\S+)/m)
{
On Mon, Nov 26, 2018 at 02:43:02PM +0900, Michael Paquier wrote:
Another option could also be to switch contribcheck and modulescheck
so as they use a temporary installation, but that's a can of worms
waiting to explode as MSVC makes more complicated the search for
initdb and such (see the way upgradecheck happens for example), and
this makes the tests waaay longer to run.
This has been itching me, and actually it proves to not be that
complicated to achieve per the attached. This makes all the tests from
contrib/ and src/test/modules pass with temporary installations, the
tests runs are much slower though. This would not blow up the buildfarm
visibly, buts its code assumes that installcheck should be used, so we
could as well just introduce new options for vcregress.pl. I am not
sure what would be the best way, still using temporary installations has
the merit to not cause any tests to run unconfigured in the future.
--
Michael
Attachments:
fix-msvc-regress-opts-v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 599b521014..5d3910b5ef 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -380,8 +380,10 @@ sub plcheck
sub subdircheck
{
+ my $subpath = shift;
my $module = shift;
+ chdir "${topdir}/${subpath}";
if ( !-d "$module/sql"
|| !-d "$module/expected"
|| (!-f "$module/GNUmakefile" && !-f "$module/Makefile"))
@@ -389,7 +391,7 @@ sub subdircheck
return;
}
- chdir $module;
+ chdir "${topdir}/${subpath}/${module}";
my @tests = fetchTests();
my @opts = fetchRegressOpts();
@@ -419,17 +421,23 @@ sub subdircheck
print "Checking $module\n";
my @args = (
"$topdir/$Config/pg_regress/pg_regress",
- "--bindir=${topdir}/${Config}/psql",
+ "--dlpath=${topdir}/src/test/regress",
+ "--bindir=",
+ "--encoding=SQL_ASCII",
+ "--no-locale",
+ "--temp-instance=./tmp_check",
+ "--inputdir=.",
"--dbname=contrib_regression", @opts, @tests);
print join(' ', @args), "\n";
system(@args);
- chdir "..";
return;
}
sub contribcheck
{
- chdir "../../../contrib";
+ InstallTemp();
+ my $subpath = "contrib";
+ chdir "${topdir}/${subpath}";
my $mstat = 0;
foreach my $module (glob("*"))
{
@@ -441,7 +449,7 @@ sub contribcheck
next if ($module =~ /_plpython$/ && !defined($config->{python}));
next if ($module eq "sepgsql");
- subdircheck($module);
+ subdircheck($subpath, $module);
my $status = $? >> 8;
$mstat ||= $status;
}
@@ -451,11 +459,13 @@ sub contribcheck
sub modulescheck
{
- chdir "../../../src/test/modules";
+ InstallTemp();
+ my $subpath = "src/test/modules";
+ chdir "${topdir}/${subpath}";
my $mstat = 0;
foreach my $module (glob("*"))
{
- subdircheck($module);
+ subdircheck($subpath, $module);
my $status = $? >> 8;
$mstat ||= $status;
}
@@ -619,13 +629,18 @@ sub fetchRegressOpts
$m =~ s{\\\r?\n}{}g;
if ($m =~ /^\s*REGRESS_OPTS\s*\+?=(.*)/m)
{
+ my @split_opts = split(/\s+/, $1);
- # Substitute known Makefile variables, then ignore options that retain
- # an unhandled variable reference. Ignore anything that isn't an
- # option starting with "--".
- @opts = grep { !/\$\(/ && /^--/ }
- map { (my $x = $_) =~ s/\Q$(top_builddir)\E/\"$topdir\"/; $x; }
- split(/\s+/, $1);
+ # Substitute known Makefile variables for each option by
+ # something which is supported in this context.
+ foreach my $opt (@split_opts)
+ {
+ # ignore empty strings
+ next if ($opt =~ /^\s*$/);
+ $opt =~ s#\$\(top_builddir\)#"$topdir"#gs;
+ $opt =~ s#\$\(top_srcdir\)#"$topdir"#gs;
+ push @opts, $opt;
+ }
}
if ($m =~ /^\s*ENCODING\s*=\s*(\S+)/m)
{
On 11/26/18 12:43 AM, Michael Paquier wrote:
Hi all,
As mentioned on this thread, I have been fighting a bit with the
buildfarm when trying to introduce new PGXS options for isolation and
TAP tests:
/messages/by-id/E1gR4D0-0002Yj-Jw@gemulon.postgresql.orgHowever it happens that we have more issues than the buildfarm failures
to begin with. One of them it related to the way REGRESS_OPTS is
defined and handled across the whole tree for MSVC.There are in the tree now a couple of paths using top_builddir or
top_srcdir:
$ git grep REGRESS_OPTS | grep top
contrib/dblink/Makefile:REGRESS_OPTS =
--dlpath=$(top_builddir)/src/test/regress
contrib/pg_stat_statements/Makefile:REGRESS_OPTS = --temp-config
$(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
src/test/modules/commit_ts/Makefile:REGRESS_OPTS =
--temp-config=$(top_srcdir)/src/test/modules/commit_ts/commit_ts.conf
src/test/modules/test_rls_hooks/Makefile:REGRESS_OPTS =
--temp-config=$(top_srcdir)/src/test/modules/test_rls_hooks/rls_hooks.confUsing top_builddir for dblink is for example fine as this needs to point
out to the place where builds of pg_regress are present. However when
it comes to configuration files we should use top_builddir, and ought to
use top_srcdir instead as VPATH would complain about things gone
missing. So the definition that we make out of it is correct. However
there is an issue with MSVC which thinks that REGRESS_OPTS should only
include top_builddir. This is actually fine per-se as all MSVC tests
run with make-installcheck, and not make-check, however I think that we
should allow top_srcdir to be switched on-the-fly as much as
top_builddir as top_srcdir could be used for something else.Another way worse issue is that MSVC scripts ignore some configuration
values because of the way values of REGRESS_OPTS are parsed. This
causes some sets of regression tests to never run on Windows. For
example, here is what the command generated for pg_stat_statements, and
what happens with it:
c:/pgbuildfarm/pgbuildroot/HEAD/pgsql.build/Release/pg_regress/pg_regress \
--bindir=c:/pgbuildfarm/pgbuildroot/HEAD/pgsql.build/Release/psql \
--dbname=contrib_regression --temp-config pg_stat_statements
[...]
============== running regression test queries ===================================
All 0 tests passed.
=====================This causes the tests to pass, but to run nothing as test list is eaten
out as an option value for --temp-config. In this case, what happens is
that --temp-config is set to "pg_stat_statements", leaving the test list
empty. The same cannot happen with test_decoding as the buildfarm uses
a custom module to run its tests (still this module could go away with
PGXS options to control isolation tests).Attached is a patch which makes MSVC scripts more intelligent by being
able to replace top_srcdir as well by topdir when fetching options.Thanks to that, MSVC is able to run the tests correctly, by building a
proper command. I think that this is a bug that should be back-patched,
because the tests just don't run, and we likely would miss regressions
because of that.Unfortunately, if I were to commit that directly, the buildfarm would
turn immediately to red for all Windows members, because when checking
module and contrib tests an "installcheck" happens, and
shared_preload_libraries is not set in this case. When working on the
switch to add isolation and TAP test support to PGXS, test_decoding has
been failing because the installed server did not have wal_level =
logical set up, which is one instance of that. The buildfarm code
installing the Postgres instance on which the tests are run should
configure that properly I think, and one tweak that we could use for the
time being is to bypass tests of modules which cannot work yet, so as
the buildfarm keeps being green. This way, this would not be a blocker
for the integration of the new PGXS infra for TAP and isolation. And
those could be enabled back once the buildfarm code is able to set up a
proper configuration. The following modules require some extra
configuration:
- commit_ts
- test_rls_hooks
- pg_stat_statements
- test_decoding (if its Makefile is rewritten)
- snapshot_too_old (if its Makefile is rewritten)The buildfarm part requires Andrew Dunstan's input, and there is a bit
to sort out for the rest, so input from all is welcome. Please note
that I would prefer if the tests which just cannot work yet are
disabled until the needed buildfarm infrastructure is needed. Another
option could also be to switch contribcheck and modulescheck so as they
use a temporary installation, but that's a can of worms waiting to
explode as MSVC makes more complicated the search for initdb and such
(see the way upgradecheck happens for example), and this makes the tests
waaay longer to run.
It's not the buildfarm that is broken. Both contribcheck() and
modulescheck() in vcregress.pl run in installcheck mode, i.e. with an
already running database. That makes the tests run faster, because
setting up and breaking down instances is expensive, especially on
Windows. We've got far too profligate with that in recent years, to the
extent that some of my Windows buildfarm animals take many hours to
complete full runs these days.
Note that TAP tests are not a problem here. It's up to the TAP scripts
to set up and break down postgres instances they need, with whatever
config options are required, such as shared preload libraries. It's only
modules using pg_regress that could be a problem.
A simple way to proceed might be to have vcregress.pl honor the
NO_INSTALLCHECK flag. Currently this is only used by pg_stat_statements,
but we could add it anywhere required. In vcregress,pl, I would probably
do this by having fetchTests() check for the flag and return an empty
set of tests if it's found, and in turn have subdircheck() do nothing if
it has an empty set of tests.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Nov 26, 2018 at 12:41:21PM -0500, Andrew Dunstan wrote:
It's not the buildfarm that is broken. Both contribcheck() and
modulescheck() in vcregress.pl run in installcheck mode, i.e. with an
already running database. That makes the tests run faster, because setting
up and breaking down instances is expensive, especially on Windows. We've
got far too profligate with that in recent years, to the extent that some of
my Windows buildfarm animals take many hours to complete full runs these
days.
I have noticed the performance impact when switching to check for those
targets. It was bad.
Note that TAP tests are not a problem here. It's up to the TAP scripts to
set up and break down postgres instances they need, with whatever config
options are required, such as shared preload libraries. It's only modules
using pg_regress that could be a problem.
Indeed.
A simple way to proceed might be to have vcregress.pl honor the
NO_INSTALLCHECK flag. Currently this is only used by pg_stat_statements, but
we could add it anywhere required. In vcregress.pl, I would probably do this
by having fetchTests() check for the flag and return an empty set of tests
if it's found, and in turn have subdircheck() do nothing if it has an empty
set of tests.
Okay, let's do so by supporting correctly NO_INSTALLCHECK. My other
refactoring work can also live with that. Returning an empty list via
fetchTests() and bypass a run if nothing is present looks fine to me.
One extra thing is that modules/commit_ts and modules/test_rls_hooks are
missing NO_INSTALLCHECK, so we would need to add that part to be
completely correct. I would also be inclined to back-patch both parts,
however for my stuff I could live with this patch only on HEAD, and
anybody willing to use installcheck on commit_ts and test_rls_hooks may
be surprised to not be able to do that anymore after the minor release.
It still looks incorrect to me though to be able to run installcheck on
those modules though...
Attached is a proposal of patch, which works as I would expect with
modulescheck and contribcheck. How does that look?
--
Michael
Attachments:
fix-msvc-no-installcheck-v3.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/test/modules/commit_ts/Makefile b/src/test/modules/commit_ts/Makefile
index 6d4f3be358..6a9c3971fb 100644
--- a/src/test/modules/commit_ts/Makefile
+++ b/src/test/modules/commit_ts/Makefile
@@ -2,6 +2,9 @@
REGRESS = commit_timestamp
REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/commit_ts/commit_ts.conf
+# Disabled because these tests require "track_commit_timestamp = on",
+# which typical installcheck users do not have (e.g. buildfarm clients).
+NO_INSTALLCHECK = 1
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/src/test/modules/test_rls_hooks/Makefile b/src/test/modules/test_rls_hooks/Makefile
index 6b772c4db1..284fdaf095 100644
--- a/src/test/modules/test_rls_hooks/Makefile
+++ b/src/test/modules/test_rls_hooks/Makefile
@@ -9,6 +9,9 @@ EXTENSION = test_rls_hooks
REGRESS = test_rls_hooks
REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_rls_hooks/rls_hooks.conf
+# Disabled because these tests require "shared_preload_libraries=test_rls_hooks",
+# which typical installcheck users do not have (e.g. buildfarm clients).
+NO_INSTALLCHECK = 1
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 599b521014..4018313bf2 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -361,6 +361,10 @@ sub plcheck
{
@lang_args = ();
}
+
+ # Move on if no tests are listed.
+ next if (scalar @tests == 0);
+
print
"============================================================\n";
print "Checking $lang\n";
@@ -391,6 +395,14 @@ sub subdircheck
chdir $module;
my @tests = fetchTests();
+
+ # Leave if no tests are listed in the module.
+ if (scalar @tests == 0)
+ {
+ chdir "..";
+ return;
+ }
+
my @opts = fetchRegressOpts();
# Special processing for python transform modules, see their respective
@@ -638,6 +650,8 @@ sub fetchRegressOpts
return @opts;
}
+# Fetch the list of tests by parsing a module's Makefile. An empty
+# list is returned if the module does not need to run anything.
sub fetchTests
{
@@ -651,6 +665,14 @@ sub fetchTests
my $t = "";
$m =~ s{\\\r?\n}{}g;
+
+ # A module specifying NO_INSTALLCHECK does not support installcheck,
+ # so bypass its run by returning an empty set of tests.
+ if ($m =~ /^\s*NO_INSTALLCHECK\s*=\s*\S+/m)
+ {
+ return ();
+ }
+
if ($m =~ /^REGRESS\s*=\s*(.*)$/gm)
{
$t = $1;
On Tue, Nov 27, 2018 at 10:27:17AM +0900, Michael Paquier wrote:
Okay, let's do so by supporting correctly NO_INSTALLCHECK. My other
refactoring work can also live with that. Returning an empty list via
fetchTests() and bypass a run if nothing is present looks fine to me.
One extra thing is that modules/commit_ts and modules/test_rls_hooks are
missing NO_INSTALLCHECK, so we would need to add that part to be
completely correct. I would also be inclined to back-patch both parts,
however for my stuff I could live with this patch only on HEAD, and
anybody willing to use installcheck on commit_ts and test_rls_hooks may
be surprised to not be able to do that anymore after the minor release.
It still looks incorrect to me though to be able to run installcheck on
those modules though...Attached is a proposal of patch, which works as I would expect with
modulescheck and contribcheck. How does that look?
If possible, I would like to move on with this stuff. To keep things
green in the buildfarm all the time, I would like to do that with two
independent steps:
1) Add NO_INSTALLCHECK to modules/commit_ts and modules/test_rls_hook.
2) Add support for NO_INSTALLCHECK in the MSVC scripts.
Are there any objections? I could do a back-patch as well to keep
things consistent across branches if there are voices in favor of that,
but that's not necessary for the upcoming Makefile cleanup with the new
set of PGXS options.
--
Michael
On 11/27/18 4:10 PM, Michael Paquier wrote:
On Tue, Nov 27, 2018 at 10:27:17AM +0900, Michael Paquier wrote:
Okay, let's do so by supporting correctly NO_INSTALLCHECK. My other
refactoring work can also live with that. Returning an empty list via
fetchTests() and bypass a run if nothing is present looks fine to me.
One extra thing is that modules/commit_ts and modules/test_rls_hooks are
missing NO_INSTALLCHECK, so we would need to add that part to be
completely correct. I would also be inclined to back-patch both parts,
however for my stuff I could live with this patch only on HEAD, and
anybody willing to use installcheck on commit_ts and test_rls_hooks may
be surprised to not be able to do that anymore after the minor release.
It still looks incorrect to me though to be able to run installcheck on
those modules though...Attached is a proposal of patch, which works as I would expect with
modulescheck and contribcheck. How does that look?If possible, I would like to move on with this stuff. To keep things
green in the buildfarm all the time, I would like to do that with two
independent steps:
1) Add NO_INSTALLCHECK to modules/commit_ts and modules/test_rls_hook.
2) Add support for NO_INSTALLCHECK in the MSVC scripts.Are there any objections? I could do a back-patch as well to keep
things consistent across branches if there are voices in favor of that,
but that's not necessary for the upcoming Makefile cleanup with the new
set of PGXS options.
I think you should just proceed with the changes above. I just had a
quick look at the patch you posted before, and it looks sane enough.
I don't see a need to backpatch.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Nov 27, 2018 at 05:59:34PM -0500, Andrew Dunstan wrote:
I think you should just proceed with the changes above. I just had a quick
look at the patch you posted before, and it looks sane enough.
Thanks for the feedback, Andrew. Let's wait a couple of days and see if
anybody has any objection about the patch set.
I don't see a need to backpatch.
Okay, no problem with that.
--
Michael