meson: Optionally disable installation of test modules

Hi,

On 2023-01-30 08:37:42 +0100, Peter Eisentraut wrote:

One open issue (IMO) with the meson build system is that it installs the
test modules under src/test/modules/ as part of a normal installation. This
is because there is no way to set up up the build system to install extra
things only when told. I think we still need a way to disable this somehow,
so that building a production installation doesn't end up with a bunch of
extra files.

The attached simple patch is a starting point for discussion. It just
disables the subdirectory src/test/modules/ based on some Boolean setting.
This could be some new top-level option, or maybe part of PG_TEST_EXTRA, or
something else? With this, I get an identical set of installed files from
meson. I imagine this option would be false by default and developers would
enable it.

Bilal, with a bit of help by me, worked on an alternative approach to
this. It's a lot more verbose in the initial change, but wouldn't increase the
amount of work/lines for new test modules. The main advantage is that we
wouldn't have disable the modules by default, which I think would be quite
likely to result in plenty people not running the tests.

Sending a link instead of attaching, in case you already registered a cfbot entry:
https://github.com/anarazel/postgres/commit/d1d192a860da39af9aa63d7edf643eed0eeee7c4

Probably worth adding an install-test-modules target for manual use.

Greetings,

Andres Freund

On 30.01.23 18:42, Andres Freund wrote:

On 2023-01-30 08:37:42 +0100, Peter Eisentraut wrote:

One open issue (IMO) with the meson build system is that it installs the
test modules under src/test/modules/ as part of a normal installation. This
is because there is no way to set up up the build system to install extra
things only when told. I think we still need a way to disable this somehow,
so that building a production installation doesn't end up with a bunch of
extra files.

The attached simple patch is a starting point for discussion. It just
disables the subdirectory src/test/modules/ based on some Boolean setting.
This could be some new top-level option, or maybe part of PG_TEST_EXTRA, or
something else? With this, I get an identical set of installed files from
meson. I imagine this option would be false by default and developers would
enable it.

Bilal, with a bit of help by me, worked on an alternative approach to
this. It's a lot more verbose in the initial change, but wouldn't increase the
amount of work/lines for new test modules. The main advantage is that we
wouldn't have disable the modules by default, which I think would be quite
likely to result in plenty people not running the tests.

Sending a link instead of attaching, in case you already registered a cfbot entry:
https://github.com/anarazel/postgres/commit/d1d192a860da39af9aa63d7edf643eed0eeee7c4

Probably worth adding an install-test-modules target for manual use.

Looks like a good idea. I'm happy to proceed along that line.

Hi,

On 1/31/23 11:44, Peter Eisentraut wrote:

On 30.01.23 18:42, Andres Freund wrote:

Bilal, with a bit of help by me, worked on an alternative approach to
this. It's a lot more verbose in the initial change, but wouldn't
increase the
amount of work/lines for new test modules. The main advantage is that we
wouldn't have disable the modules by default, which I think would be
quite
likely to result in plenty people not running the tests.

Sending a link instead of attaching, in case you already registered a
cfbot entry:
https://github.com/anarazel/postgres/commit/d1d192a860da39af9aa63d7edf643eed0eeee7c4

Probably worth adding an install-test-modules target for manual use.

Looks like a good idea.  I'm happy to proceed along that line.

I am adding a patch of an alternative approach. Also, I updated the
commit at the link by adding regress_module, autoinc_regress and
refint_regress to the test_install_libs.

Regards,
Nazir Bilal Yavuz
Microsoft

On 01.02.23 13:41, Nazir Bilal Yavuz wrote:

On 1/31/23 11:44, Peter Eisentraut wrote:

On 30.01.23 18:42, Andres Freund wrote:

Bilal, with a bit of help by me, worked on an alternative approach to
this. It's a lot more verbose in the initial change, but wouldn't
increase the
amount of work/lines for new test modules. The main advantage is that we
wouldn't have disable the modules by default, which I think would be
quite
likely to result in plenty people not running the tests.

Sending a link instead of attaching, in case you already registered a
cfbot entry:
https://github.com/anarazel/postgres/commit/d1d192a860da39af9aa63d7edf643eed0eeee7c4

Probably worth adding an install-test-modules target for manual use.

Looks like a good idea.  I'm happy to proceed along that line.

I am adding a patch of an alternative approach. Also, I updated the
commit at the link by adding regress_module, autoinc_regress and
refint_regress to the test_install_libs.

If you feel that your patch is ready, please add it to the commit fest.
I look forward to reviewing it.

Hi,

On 2/8/23 13:30, Peter Eisentraut wrote:

If you feel that your patch is ready, please add it to the commit
fest. I look forward to reviewing it.

Thanks! Commit fest entry link: https://commitfest.postgresql.org/42/4173/

Regards,
Nazir Bilal Yavuz
Microsoft

On 09.02.23 16:30, Nazir Bilal Yavuz wrote:

On 2/8/23 13:30, Peter Eisentraut wrote:

If you feel that your patch is ready, please add it to the commit
fest. I look forward to reviewing it.

Thanks! Commit fest entry link: https://commitfest.postgresql.org/42/4173/

I tested this a bit. It works fine. The approach makes sense to me.

The install_additional_files script could be simplified a bit. You
could use os.makedirs(dest, exist_ok=True) and avoid the error checking.
I don't think any callers try to copy a directory source, so the
shutil.copytree() stuff isn't necessary. Run pycodestyle over the
script. And let's put the script into src/tools/ like the other support
scripts.

On 2023-02-20 19:43:56 +0100, Peter Eisentraut wrote:

I don't think any callers try to copy a directory source, so the
shutil.copytree() stuff isn't necessary.

I'd like to use it for installing docs outside of the normal install
target. Of course we could add the ability at a later point, but that seems a
bit pointless back-forth to me.

On 20.02.23 20:48, Andres Freund wrote:

On 2023-02-20 19:43:56 +0100, Peter Eisentraut wrote:

I don't think any callers try to copy a directory source, so the
shutil.copytree() stuff isn't necessary.

I'd like to use it for installing docs outside of the normal install
target. Of course we could add the ability at a later point, but that seems a
bit pointless back-forth to me.

I figured it could be useful as a general installation tool, but the
current script has specific command-line options for this specific
purpose, so I don't think it would work for your purpose anyway.

For the purpose here, we really just need something that does

for src in sys.argv[1:-1]:
shutil.copy2(src, sys.argv[-1])

But we need to call it twice for different sets of files and
destinations, and since we can't have more than one command per test, we
either need to write two "tests" or write a wrapper script like the one
we have here.

I don't know what the best way to slice this is, but it's not a lot of
code that we couldn't move around again in the future.

Hi,

Thanks for the review.

On Mon, 20 Feb 2023 at 21:44, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

I tested this a bit. It works fine. The approach makes sense to me.

The install_additional_files script could be simplified a bit. You
could use os.makedirs(dest, exist_ok=True) and avoid the error checking.
I don't think any callers try to copy a directory source, so the
shutil.copytree() stuff isn't necessary. Run pycodestyle over the
script. And let's put the script into src/tools/ like the other support
scripts.

I updated the patch in line with your comments.

Regards,
Nazir Bilal Yavuz
Microsoft

Hi,

On 2023-02-22 10:09:10 +0100, Peter Eisentraut wrote:

On 20.02.23 20:48, Andres Freund wrote:

On 2023-02-20 19:43:56 +0100, Peter Eisentraut wrote:

I don't think any callers try to copy a directory source, so the
shutil.copytree() stuff isn't necessary.

I'd like to use it for installing docs outside of the normal install
target. Of course we could add the ability at a later point, but that seems a
bit pointless back-forth to me.

I figured it could be useful as a general installation tool, but the current
script has specific command-line options for this specific purpose, so I
don't think it would work for your purpose anyway.

For the purpose here, we really just need something that does

for src in sys.argv[1:-1]:
shutil.copy2(src, sys.argv[-1])

But we need to call it twice for different sets of files and destinations,
and since we can't have more than one command per test, we either need to
write two "tests" or write a wrapper script like the one we have here.

How about making the arguments
--install target-path list of files or directories
--install another-path another set of files

I don't know what the best way to slice this is, but it's not a lot of code
that we couldn't move around again in the future.

That's true. The main work here is going through all the test modules, and
that won't be affected by changing the argument syntax.

Greetings,

Andres Freund

On 23.02.23 19:06, Nazir Bilal Yavuz wrote:

Hi,

Thanks for the review.

On Mon, 20 Feb 2023 at 21:44, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

I tested this a bit. It works fine. The approach makes sense to me.

The install_additional_files script could be simplified a bit. You
could use os.makedirs(dest, exist_ok=True) and avoid the error checking.
I don't think any callers try to copy a directory source, so the
shutil.copytree() stuff isn't necessary. Run pycodestyle over the
script. And let's put the script into src/tools/ like the other support
scripts.

I updated the patch in line with your comments.

Looks good to me. I did a small pass over it to adjust some namings.
For example, I renamed test_install_files to test_install_data, so it's
consistent with the overall meson naming:

-install_data(
+test_install_data += files(

Let me know if you have any concerns about this version. Otherwise, I'm
happy to commit it.

Hi,

On Wed, 1 Mar 2023 at 22:21, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

Looks good to me. I did a small pass over it to adjust some namings.
For example, I renamed test_install_files to test_install_data, so it's
consistent with the overall meson naming:

-install_data(
+test_install_data += files(

Let me know if you have any concerns about this version. Otherwise, I'm
happy to commit it.

That makes sense, thanks!

Regards,
Nazir Bilal Yavuz
Microsoft

On 02.03.23 08:09, Nazir Bilal Yavuz wrote:

On Wed, 1 Mar 2023 at 22:21, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

Looks good to me. I did a small pass over it to adjust some namings.
For example, I renamed test_install_files to test_install_data, so it's
consistent with the overall meson naming:

-install_data(
+test_install_data += files(

Let me know if you have any concerns about this version. Otherwise, I'm
happy to commit it.

That makes sense, thanks!

committed

On 2023-03-03 Fr 01:47, Peter Eisentraut wrote:

On 02.03.23 08:09, Nazir Bilal Yavuz wrote:

On Wed, 1 Mar 2023 at 22:21, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

Looks good to me.  I did a small pass over it to adjust some namings.
For example, I renamed test_install_files to test_install_data, so it's
consistent with the overall meson naming:

-install_data(
+test_install_data += files(

Let me know if you have any concerns about this version. Otherwise, I'm
happy to commit it.

That makes sense, thanks!

committed

These changes have broken the buildfarm adaptation work in different
ways on different platforms.

On Windows (but not Linux), the install_test_files are apparently
getting installed under runpython in the build directory rather than in
the tmp_install location, so those tests fail. Meanwhile, it's not clear
to me how to install them in a standard installation, which means that
on Linux the corresponding -running tests are failing.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Hi,

On Fri, 3 Mar 2023 at 22:43, Andrew Dunstan <andrew@dunslane.net> wrote:

These changes have broken the buildfarm adaptation work in different ways on different platforms.

On Windows (but not Linux), the install_test_files are apparently getting installed under runpython in the build directory rather than in the tmp_install location, so those tests fail. Meanwhile, it's not clear to me how to install them in a standard installation, which means that on Linux the corresponding -running tests are failing.

Is there a way to see the 'meson-logs/testlog.txt' file under the
build directory?

Regards,
Nazir Bilal Yavuz
Microsoft

On 2023-03-06 Mo 08:47, Nazir Bilal Yavuz wrote:

Hi,

On Fri, 3 Mar 2023 at 22:43, Andrew Dunstan<andrew@dunslane.net> wrote:

These changes have broken the buildfarm adaptation work in different ways on different platforms.

On Windows (but not Linux), the install_test_files are apparently getting installed under runpython in the build directory rather than in the tmp_install location, so those tests fail. Meanwhile, it's not clear to me how to install them in a standard installation, which means that on Linux the corresponding -running tests are failing.

Is there a way to see the 'meson-logs/testlog.txt' file under the
build directory?

There are two separate issues here, but let's deal with the Windows
issue. Attached is the log output and also a listing of the runpython
directory in the build directory.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Hi,

On Mon, 6 Mar 2023 at 18:30, Andrew Dunstan <andrew@dunslane.net> wrote:

There are two separate issues here, but let's deal with the Windows issue. Attached is the log output and also a listing of the runpython directory in the build directory.

Thanks for the logs but I couldn't understand the problem. Is there a
way to reproduce this?

For the Linux problem, Andres's patch solves this but you need to run
an extra command. [1]/messages/by-id/20230308012940.edexipb3vqylcu6r@awork3.anarazel.de

After applying Andres's patch, you need to run:
$ meson compile install-test-files -C $pgsql
before running the 'running tests'.

I tested on my local and
......
$ meson compile install-test-files -C $pgsql
$ meson test -C $pgsql --setup running --print-errorlogs --no-rebuild
--logbase installcheckworld --no-suite regress-running --no-suite
isolation-running --no-suite ecpg-running
passed successfully.

[1]: /messages/by-id/20230308012940.edexipb3vqylcu6r@awork3.anarazel.de

Regards,
Nazir Bilal Yavuz
Microsoft

On 2023-03-08 We 08:49, Nazir Bilal Yavuz wrote:

Hi,

On Mon, 6 Mar 2023 at 18:30, Andrew Dunstan<andrew@dunslane.net> wrote:

There are two separate issues here, but let's deal with the Windows issue. Attached is the log output and also a listing of the runpython directory in the build directory.

Thanks for the logs but I couldn't understand the problem. Is there a
way to reproduce this?

Problem now apparently resolved.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com