Horton

Thread

Commits

  1. meson: Prevent installation of test files during main install

  1. meson: Optionally disable installation of test modules

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-30T07:37:42Z

    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?

  2. Re: meson: Optionally disable installation of test modules

    Andres Freund <andres@anarazel.de> — 2023-01-30T17:42:14Z

    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

  3. Re: meson: Optionally disable installation of test modules

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-31T08:44:03Z

    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.

  4. Re: meson: Optionally disable installation of test modules

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2023-02-01T12:41:21Z

    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

  5. Re: meson: Optionally disable installation of test modules

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-02-08T10:30:10Z

    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.

  6. Re: meson: Optionally disable installation of test modules

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2023-02-09T15:30:20Z

    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

  7. Re: meson: Optionally disable installation of test modules

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-02-20T18:43:56Z

    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.

  8. Re: meson: Optionally disable installation of test modules

    Andres Freund <andres@anarazel.de> — 2023-02-20T19:48:20Z

    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.

  9. Re: meson: Optionally disable installation of test modules

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-02-22T09:09:10Z

    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.

  10. Re: meson: Optionally disable installation of test modules

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2023-02-23T18:06:26Z

    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

  11. Re: meson: Optionally disable installation of test modules

    Andres Freund <andres@anarazel.de> — 2023-02-25T18:36:12Z

    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

  12. Re: meson: Optionally disable installation of test modules

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-03-01T19:20:58Z

    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.

  13. Re: meson: Optionally disable installation of test modules

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2023-03-02T07:09:48Z

    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

  14. Re: meson: Optionally disable installation of test modules

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-03-03T06:47:03Z

    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

  15. Re: meson: Optionally disable installation of test modules

    Andrew Dunstan <andrew@dunslane.net> — 2023-03-03T19:43:25Z

    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

  16. Re: meson: Optionally disable installation of test modules

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2023-03-06T13:47:03Z

    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

  17. Re: meson: Optionally disable installation of test modules

    Andrew Dunstan <andrew@dunslane.net> — 2023-03-06T15:30:22Z

    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

  18. Re: meson: Optionally disable installation of test modules

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2023-03-08T13:49:37Z

    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]

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

  19. Re: meson: Optionally disable installation of test modules

    Andrew Dunstan <andrew@dunslane.net> — 2023-03-08T22:25:52Z

    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