Hi,
On Wed, 11 Sept 2024 at 13:04, Ashutosh Bapat
<ashutosh.bapat....@gmail.com> wrote:
>
> Thanks Bilal for testing the patch. Can you or Jacob please create one
> patchset including both meson and make fixes? Please keep the meson
> and make changes in separate patches though. I think the meson patches
> come from [1] (they probably need a rebase, git am failed) and make
> patch comes from [2].The one fixing make needs a good commit message.
I created and attached a patchset and wrote a commit message to 'make'
fix. Please feel free to edit them.
> some comments on code
>
> 1. comments on changes to meson
>
> + variable if it exists. See <xref linkend="regress-additional"/> for
>
> s/exists/set/
Done.
> -# Test suites that are not safe by default but can be run if selected
> -# by the user via the whitespace-separated list in variable PG_TEST_EXTRA.
> -# Export PG_TEST_EXTRA so it can be checked in individual tap tests.
> -test_env.set('PG_TEST_EXTRA', get_option('PG_TEST_EXTRA'))
>
> A naive question. What happens if we add PG_TEST_EXTRA in meson.build
> itself rather than passing it via testwrap? E.g. like
> if "PG_TEST_EXTRA" not in os.environ
> test_env.set('PG_TEST_EXTRA', get_option('PG_TEST_EXTRA'))
Then this configure time option will be passed to the test environment
and there is no way to change it without reconfiguring if we don't
touch the testwrap file.
> I am worried that we might have to add an extra argument to testwrap
> for every environment variable that influences the tests. Avoiding it
> would be better.
If we want to have both configure time and run time variables, I
believe that this is the only way for now.
> option('PG_TEST_EXTRA', type: 'string', value: '',
> - description: 'Enable selected extra tests')
> + description: 'Enable selected extra tests, please note that this
> configure option is overridden by PG_TEST_EXTRA environment variable
> if it exists')
>
> All the descriptions look much shorter than this one. I suggest we
> shorten this one too as
> "Enable selected extra tests. Overridden by PG_TEST_EXTRA environment
> variable."
> not as short as other descriptions but shorter than before and yet
> serves its intended purpose. Or just make it the same as the one in
> configure.ac. Either way the descriptions in configure.ac and
> meson_options.txt should be in sync.
I liked your suggestion, done in both meson_options.txt and configure.ac.
> +# If PG_TEST_EXTRA is not set in the environment, then look for its Meson
> +# configure option.
> +if "PG_TEST_EXTRA" not in os.environ and args.pg_test_extra:
> + env_dict["PG_TEST_EXTRA"] = args.pg_test_extra
> +
>
> If somebody looks at just these lines, it's not clear how
> PG_TEST_EXTRA is passed to the test environment if it's available in
> os.environ. So one has to understand that env_dict is the test
> environment. If that's so, the code and comment rewritten as below
> makes more sense to me. What do you think?
>
> # If PG_TEST_EXTRA is not already part of the test environment, check if it's
> # passed via program argument --pg_test_extra. Thus we also override
> # configuration time value by run time value of PG_TEST_EXTRA.
> if "PG_TEST_EXTRA" not in env_dict and args.pg_test_extra:
> env_dict["PG_TEST_EXTRA"] = args.pg_test_extra
I think your suggestion is better, done.
> But in case we decide to fix meson.build as suggested in one of the
> commentsabove, this change will be unnecessary.
>
> Note that PG_TEST_EXTRA is used in only TAP tests right now. The way
> the value passed to --pg_test_extra is handled in testwrap, it will
> available to every test, not just TAP tests. But this looks fine to me
> since the documentation of PG_TEST_EXTRA or its name itself does not
> show any intention to limit it only to TAP tests.
I agree, it looks fine to me as well.
> 2. comments on make changes
> Since Makefile.global.in is included in src/test/Makefile I was
> expecting that the PG_TEST_EXTRA picked from configuration would be
> available in src/test/Makefile from which it would be exported. But
> that doesn't seem to be the case. In fact, I am getting doubtful about
> the role of the current "export PG_TEST_EXTRA" in /src/test/Makefile.
> Even if I remove it, it doesn't affect anything. Commands a.
> PG_TEST_EXTRA=xid_wraparound make check, b.
> PG_TEST_EXTRA=xid_wraparound make -C $XID_MODULE_DIR check run the
> tests (and don't skip them).
Yes, it looks like it is useless. If we export PG_TEST_EXTRA, then it
should be already available on the environment, right?
> Anyway with the proposed change PG_TEST_EXTRA passed at configuration
> time is used if it's not defined at run time as expected. I think the
> patch looks good. Nothing to complain there.
--
Regards,
Nazir Bilal Yavuz
Microsoft
From 3a511b2a287b7660a0be1751fda8b52f1f5995cc Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <byavu...@gmail.com>
Date: Wed, 11 Sep 2024 15:41:27 +0300
Subject: [PATCH v4 1/2] Make PG_TEST_EXTRA env variable override its Meson
configure option
Since PG_TEST_EXTRA configure option is set while configuring Meson,
each change on PG_TEST_EXTRA needs reconfigure and this is costly. So,
first try to use PG_TEST_EXTRA from environment. If it does not exist,
then try to use it from its configure option in Meson builds.
After the above change, Meson builds are able to get PG_TEST_EXTRA from
environment and this overrides the configure option. PG_TEST_EXTRA
environment variable is set at the top of the .cirrus.tasks.yml file. So,
PG_TEXT_EXTRA in configure scripts became useless and were removed
because of that.
---
doc/src/sgml/installation.sgml | 6 ++++--
.cirrus.tasks.yml | 6 +-----
meson.build | 11 ++++++-----
meson_options.txt | 2 +-
src/tools/testwrap | 8 ++++++++
5 files changed, 20 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index ff9abd4649d..7c481e07e98 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -3073,8 +3073,10 @@ ninja install
<listitem>
<para>
Enable test suites which require special software to run. This option
- accepts arguments via a whitespace-separated list. See <xref
- linkend="regress-additional"/> for details.
+ accepts arguments via a whitespace-separated list. Please note that
+ this configure option is overridden by PG_TEST_EXTRA environment
+ variable if it is set. See <xref linkend="regress-additional"/> for
+ details.
</para>
</listitem>
</varlistentry>
diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index 90cb95c8681..fc413eb11ef 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -175,7 +175,6 @@ task:
--buildtype=debug \
-Dcassert=true -Dinjection_points=true \
-Duuid=bsd -Dtcl_version=tcl86 -Ddtrace=auto \
- -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
-Dextra_lib_dirs=/usr/local/lib -Dextra_include_dirs=/usr/local/include/ \
build
EOF
@@ -364,7 +363,6 @@ task:
--buildtype=debug \
-Dcassert=true -Dinjection_points=true \
${LINUX_MESON_FEATURES} \
- -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
build
EOF
@@ -380,7 +378,6 @@ task:
-Dllvm=disabled \
--pkg-config-path /usr/lib/i386-linux-gnu/pkgconfig/ \
-DPERL=perl5.36-i386-linux-gnu \
- -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
build-32
EOF
@@ -502,7 +499,6 @@ task:
-Dextra_lib_dirs=/opt/local/lib \
-Dcassert=true -Dinjection_points=true \
-Duuid=e2fs -Ddtrace=auto \
- -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
build
build_script: ninja -C build -j${BUILD_JOBS}
@@ -574,7 +570,7 @@ task:
# Use /DEBUG:FASTLINK to avoid high memory usage during linking
configure_script: |
vcvarsall x64
- meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Dinjection_points=true -Db_pch=true -Dextra_lib_dirs=c:\openssl\1.1\lib -Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR% -DPG_TEST_EXTRA="%PG_TEST_EXTRA%" build
+ meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Dinjection_points=true -Db_pch=true -Dextra_lib_dirs=c:\openssl\1.1\lib -Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR% build
build_script: |
vcvarsall x64
diff --git a/meson.build b/meson.build
index 4764b09266e..c11bc928b93 100644
--- a/meson.build
+++ b/meson.build
@@ -3322,11 +3322,6 @@ test_env.set('PG_REGRESS', pg_regress.full_path())
test_env.set('REGRESS_SHLIB', regress_module.full_path())
test_env.set('INITDB_TEMPLATE', test_initdb_template)
-# Test suites that are not safe by default but can be run if selected
-# by the user via the whitespace-separated list in variable PG_TEST_EXTRA.
-# Export PG_TEST_EXTRA so it can be checked in individual tap tests.
-test_env.set('PG_TEST_EXTRA', get_option('PG_TEST_EXTRA'))
-
# Add the temporary installation to the library search path on platforms where
# that works (everything but windows, basically). On windows everything
# library-like gets installed into bindir, solving that issue.
@@ -3390,6 +3385,12 @@ foreach test_dir : tests
testwrap,
'--basedir', meson.build_root(),
'--srcdir', test_dir['sd'],
+ # Test suites that are not safe by default but can be run if selected
+ # by the user via the whitespace-separated list in variable PG_TEST_EXTRA.
+ # Export PG_TEST_EXTRA so it can be checked in individual tap tests.
+ # This configure option is overridden by PG_TEST_EXTRA environment variable
+ # if it exists.
+ '--pg_test_extra', get_option('PG_TEST_EXTRA'),
]
foreach kind, v : test_dir
diff --git a/meson_options.txt b/meson_options.txt
index b9421557606..38935196394 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -47,7 +47,7 @@ option('injection_points', type: 'boolean', value: false,
description: 'Enable injection points')
option('PG_TEST_EXTRA', type: 'string', value: '',
- description: 'Enable selected extra tests')
+ description: 'Enable selected extra tests. Overridden by PG_TEST_EXTRA environment variable.')
option('PG_GIT_REVISION', type: 'string', value: 'HEAD',
description: 'git revision to be packaged by pgdist target')
diff --git a/src/tools/testwrap b/src/tools/testwrap
index 9a270beb72d..665e3f14ed0 100755
--- a/src/tools/testwrap
+++ b/src/tools/testwrap
@@ -13,6 +13,7 @@ parser.add_argument('--basedir', help='base directory of test', type=str)
parser.add_argument('--testgroup', help='test group', type=str)
parser.add_argument('--testname', help='test name', type=str)
parser.add_argument('--skip', help='skip test (with reason)', type=str)
+parser.add_argument('--pg_test_extra', help='extra tests', type=str)
parser.add_argument('test_command', nargs='*')
args = parser.parse_args()
@@ -41,6 +42,13 @@ env_dict = {**os.environ,
'TESTDATADIR': os.path.join(testdir, 'data'),
'TESTLOGDIR': os.path.join(testdir, 'log')}
+
+# If PG_TEST_EXTRA is not already part of the test environment, check if it's
+# passed via program argument --pg_test_extra. Thus we also override
+# configuration time value by run time value of PG_TEST_EXTRA.
+if "PG_TEST_EXTRA" not in os.environ and args.pg_test_extra:
+ env_dict["PG_TEST_EXTRA"] = args.pg_test_extra
+
sp = subprocess.Popen(args.test_command, env=env_dict, stdout=subprocess.PIPE)
# Meson categorizes a passing TODO test point as bad
# (https://github.com/mesonbuild/meson/issues/13183). Remove the TODO
--
2.45.2
From 81f27b15bf1e03335afbbe0c07187474997d3aa0 Mon Sep 17 00:00:00 2001
From: Jacob Champion <jacob.champ...@enterprisedb.com>
Date: Wed, 11 Sep 2024 15:46:33 +0300
Subject: [PATCH v4 2/2] Add PG_TEST_EXTRA configure option to the make builds
The Meson builds have PG_TEST_EXTRA as a configure-time option, which was
not available in the make builds. To ensure both build systems are in
sync, PG_TEST_EXTRA is now added as a configure-time option. It can be
set like this:
./configure PG_TEST_EXTRA="kerberos, ssl, ..."
Note that to preserve the old behavior, this configure-time option is
overridden by the PG_TEST_EXTRA environment variable.
---
src/test/Makefile | 5 -----
configure | 5 +++++
configure.ac | 1 +
src/Makefile.global.in | 10 ++++++++++
4 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/src/test/Makefile b/src/test/Makefile
index dbd3192874d..fc42f1db2b9 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -27,11 +27,6 @@ ifeq ($(with_ssl),openssl)
SUBDIRS += ssl
endif
-# Test suites that are not safe by default but can be run if selected
-# by the user via the whitespace-separated list in variable PG_TEST_EXTRA.
-# Export PG_TEST_EXTRA to check it in individual tap tests.
-export PG_TEST_EXTRA
-
# We don't build or execute these by default, but we do want "make
# clean" etc to recurse into them. (We must filter out those that we
# have conditionally included into SUBDIRS above, else there will be
diff --git a/configure b/configure
index 53c8a1f2bad..d06ded9cd4a 100755
--- a/configure
+++ b/configure
@@ -764,6 +764,7 @@ LDFLAGS
CFLAGS
CC
enable_injection_points
+PG_TEST_EXTRA
enable_tap_tests
enable_dtrace
DTRACEFLAGS
@@ -880,6 +881,7 @@ enable_largefile
ac_precious_vars='build_alias
host_alias
target_alias
+PG_TEST_EXTRA
CC
CFLAGS
LDFLAGS
@@ -1587,6 +1589,8 @@ Optional Packages:
--with-openssl obsolete spelling of --with-ssl=openssl
Some influential environment variables:
+ PG_TEST_EXTRA
+ enable selected extra tests
CC C compiler command
CFLAGS C compiler flags
LDFLAGS linker flags, e.g. -L<lib dir> if you have libraries in a
@@ -3629,6 +3633,7 @@ fi
+
#
# Injection points
#
diff --git a/configure.ac b/configure.ac
index 6a35b2880bf..c5037f71964 100644
--- a/configure.ac
+++ b/configure.ac
@@ -236,6 +236,7 @@ AC_SUBST(enable_dtrace)
PGAC_ARG_BOOL(enable, tap-tests, no,
[enable TAP tests (requires Perl and IPC::Run)])
AC_SUBST(enable_tap_tests)
+AC_ARG_VAR(PG_TEST_EXTRA, [Enable selected extra tests. Overridden by PG_TEST_EXTRA environment variable.])
#
# Injection points
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 42f50b49761..4859343153b 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -642,6 +642,16 @@ submake-libpgfeutils: | submake-generated-headers
#
# Testing support
+# Store any configure-time setting for PG_TEST_EXTRA, but let environment
+# variables override it to maintain the historical behavior of the tests.
+# (Standard `=` assignment would require devs to use a commandline option.)
+# This is skipped in PGXS mode to keep the setting from escaping into other
+# projects' builds.
+ifndef PGXS
+PG_TEST_EXTRA ?= @PG_TEST_EXTRA@
+export PG_TEST_EXTRA
+endif
+
ifneq ($(USE_MODULE_DB),)
PL_TESTDB = pl_regression_$(NAME)
ifneq ($(MODULE_big),)
--
2.45.2
