Re: PG_TEST_EXTRA and meson

Hi,

On Wed, 11 Sept 2024 at 13:04, Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)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