Re: PG_TEST_EXTRA and meson

On Mon, Sep 2, 2024 at 8:32 PM Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Fri, 30 Aug 2024 at 21:36, Jacob Champion
> <jacob(dot)champion(at)enterprisedb(dot)com> wrote:
> >
> > On Wed, Aug 28, 2024 at 8:21 AM Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> wrote:
> > > I do not exactly remember the reason but I think I copied the same
> > > behavior as before, PG_TEST_EXTRA variable was checked in the
> > > src/test/Makefile so I exported it there.
> >
> > Okay, give v3 a try then. This exports directly from Makefile.global.
> > Since that gets pulled into a bunch of places, the scope is a lot
> > wider than it used to be;

This looks similar to what meson does with PG_TEST_EXTRA, it is
available via get_option(). So we are closing the gap between meson
and make. That's the intention.
> I've disabled it for PGXS so it doesn't end
> > up poisoning other extensions.
>
> Patch looks good and it passes all the test cases in Ashutosh's test script.
>

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.

some comments on code

1. comments on changes to meson

+ variable if it exists. See <xref linkend="regress-additional"/> for

s/exists/set/

-# 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'))

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.

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.

+# 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

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.

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).

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.

[1] https://www.postgresql.org/message-id/CAN55FZ1Tko2N=X4f6icgFhb7syJYo_APP-9EbFcT-uH6tEi_Xg@mail.gmail.com
[2] https://www.postgresql.org/message-id/CAOYmi+=6HNVhbFOVsV6X2_DVDYcUDL4AMnj7iM15gAfw__beKA@mail.gmail.com

--
Best Wishes,
Ashutosh Bapat