Handling of REGRESS_OPTS in MSVC for regression tests

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Postgres hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Handling of REGRESS_OPTS in MSVC for regression tests
Date: 2018-11-26 05:43:02
Message-ID: 20181126054302.GI1776@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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:
https://www.postgresql.org/message-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

Attachment Content-Type Size
fix-msvc-regress-opts.patch text/x-diff 1008 bytes

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Laurenz Albe 2018-11-26 05:44:33 Re: Updated backup APIs for non-exclusive backups
Previous Message Laurenz Albe 2018-11-26 05:31:44 Re: Updated backup APIs for non-exclusive backups