Thread: meson: Optionally disable installation of test modules
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. Thoughts?
Attachment
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
Attachment
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
Attachment
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.
Attachment
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 directoryrather than in the tmp_install location, so those tests fail. Meanwhile, it's not clear to me how to install themin 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
Attachment
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 listingof 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] 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] https://www.postgresql.org/message-id/20230308012940.edexipb3vqylcu6r%40awork3.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