Hi Nazir, On Fri, Jul 19, 2024 at 1:37 PM Nazir Bilal Yavuz <byavu...@gmail.com> wrote:
> > > > If you are willing to work on this further, please add it to the commitfest. > > Since general consensus is more towards having an environment variable > to override Meson configure option, I converted solution-3 to > something more like a patch. I updated the docs, added more comments > and added this to the commitfest [1]. Thanks. > > The one downside of this approach is that PG_TEXT_EXTRA in user > defined options in meson setup output could be misleading now. > Upthread Tom asked whether we should do a symmetric change to "make". This set of patches does not do that. Here are my thoughts: 1. Those who use make, are not using configure time PG_TEST_EXTRA anyway, so they don't need it. 2. Those meson users who use setup time PG_TEST_EXTRA and also want to use make may find the need for the feature in make. 3. https://www.postgresql.org/docs/devel/install-requirements.html says that the meson support is currently experimental and only works when building from a Git checkout. So there's a possibility (even if theoretical) that make and meson will co-exist. Also that we may abandon meson? Considering those, it seems to me that symmetry is required. I don't know how hard it is to introduce PG_TEST_EXTRA as a configure time option in "make". If it's simple, we should do that. Otherwise it will be better to just remove PG_EXTRA_TEST option support from meson support to keep make and meson symmetrical. As far as the implementation is concerned the patch seems to be doing what's expected. If PG_TEST_EXTRA is specified at setup time, it is not needed to be specified as an environment variable at run time. But it can also be overridden at runtime. If PG_TEST_EXTRA is not specified at the time of setup, but specified at run time, it works. I have tested xid_wraparound and wal_consistency_check. I wonder whether we really require pg_test_extra argument to testwrap. Why can't we use the logic in testwrap, to set run time PG_TEST_EXTRA, in meson.build directly? I.e. set test_env['PG_TEST_EXTRA'] to os.environ[;PG_TEST_EXTRA'] if the latter is set, otherwise set the first to get_option('PG_TEST_EXTRA'). > Also, with this change; PG_TEST_EXTRA from configure_scripts in the > .cirrus.tasks.yml file should be removed as they are useless now. I > added that as a second patch. I think this is useful and allows both make and meson to use the same logic in cirrus. -- Best Wishes, Ashutosh Bapat