Обсуждение: Meson build updates

Hi,

I took some time to look at the Meson build for Postgres. I contribute
some of the time to Meson, itself. Within this patchset you will find
pretty small changes. Most of the commits are attempting to create more
consistency with the surrounding code. I think there is more that can be
done to improve the build a bit like including subprojects for optional
dependencies like lz4, zstd, etc.

While I was reading through the code, I also noticed a few XXX/FIXMEs. I
don't mind taking a look at those in the future, but I think I need more
context for almost all of them since I am brand new to Postgres
development. Since I also have _some_ sway in the Meson community, I can
help get more attention to Meson issues that benefit Postgres. I
currently note 3 Meson issues that are commented in the code for
instance. If anyone ever has any problems or questions about Meson or
specifically the Meson build of Postgres, I am more than happy to
receive an email to see if I can help out.

Highlighting the biggest changes in this patchset:

- Add Meson overrides
- Remove Meson program options for specifying paths

Everything but the last patch could most likely be backported to
previous maintained releases including Meson, though the patchset is
initially targeting the master branch.

--
Tristan Partin
Neon (https://neon.tech)

Received a review from a Meson maintainer. Here is a v2.

--
Tristan Partin
Neon (https://neon.tech)

Hi,

On 2023-05-18 10:36:59 -0500, Tristan Partin wrote:
> I took some time to look at the Meson build for Postgres. I contribute
> some of the time to Meson, itself. Within this patchset you will find
> pretty small changes. Most of the commits are attempting to create more
> consistency with the surrounding code. I think there is more that can be
> done to improve the build a bit like including subprojects for optional
> dependencies like lz4, zstd, etc.

Thanks for looking over these!



> From b35ecb2c8dcd71608f98af1e0ec19d965099ceab Mon Sep 17 00:00:00 2001
> From: Tristan Partin <tristan@neon.tech>
> Date: Wed, 17 May 2023 09:40:02 -0500
> Subject: [PATCH postgres v1 12/17] Make finding pkg-config(python3) more
>  robust
> 
> It is a possibility that the installation can't be found. Checking for
> Python.h is redundant with what Meson does internally.

That's not what I saw - we had cases where Python.h was missing, but the
python dependency was found. It's possible that this is dependent on the
meson version.


> From 47394ffd113d4170e955bc033841cb7e18fd3ac4 Mon Sep 17 00:00:00 2001
> From: Tristan Partin <tristan@neon.tech>
> Date: Wed, 17 May 2023 09:44:49 -0500
> Subject: [PATCH postgres v1 14/17] Reduce branching on Meson version
> 
> This code had a branch depending on Meson version. Instead, we can just
> move the system checks to the if statement. I believe this also keeps
> selinux and systemd from being looked for on non-Linux systems when
> using Meson < 0.59. Before they would be checked, but obviously fail.

I like the current version better - depending on the meson version makes it
easy to find what needs to be removed when we upgrade the meson minimum
version.



> From 189d3ac3d5593ce3e475813e4830a29bb4e96f70 Mon Sep 17 00:00:00 2001
> From: Tristan Partin <tristan@neon.tech>
> Date: Wed, 17 May 2023 10:36:52 -0500
> Subject: [PATCH postgres v1 16/17] Add Meson overrides
> 
> Meson has the ability to do transparent overrides when projects are used
> as subprojects. For instance, say I am building a Postgres extension. I
> can define Postgres to be a subproject of my extension given the
> following wrap file:
> 
> [wrap-git]
> url = https://git.postgresql.org/git/postgresql.git
> revision = master
> depth = 1
> 
> [provide]
> dependency_names = libpq
> 
> Then in my extension (root project), I can have the following line
> snippet:
> 
> libpq = dependency('libpq')
> 
> This will tell Meson to transparently compile libpq prior to it
> compiling my extension (because I depend on libpq) if libpq isn't found
> on the host system.
> 
> I have also added overrides for the various public-facing exectuables.
> Though I don't expect them to get much usage, might as well go ahead and
> override them. They can be used by adding the following line to the
> aforementioned wrap file:

I think adding more boilerplate to all these places does have some cost. So
I'm not really convinced this is worth doign.




> From 5ee13f09e4101904dbc9887bd4844eb5f1cb4fea Mon Sep 17 00:00:00 2001
> From: Tristan Partin <tristan@neon.tech>
> Date: Wed, 17 May 2023 10:54:53 -0500
> Subject: [PATCH postgres v1 17/17] Remove Meson program options for specifying
>  paths
> 
> Meson has a built-in way to override paths without polluting project
> build options called machine files.

I think meson machine files are just about unusable. For one, you can't
actually change any of the options without creating a new build dir. For
another, it's not something that easily can be done on the commandline.

So I really don't want to remove these options.

Greetings,

Andres Freund

On Fri Jun 9, 2023 at 11:41 AM CDT, Andres Freund wrote:
> Hi,
>
> On 2023-05-18 10:36:59 -0500, Tristan Partin wrote:
> > From b35ecb2c8dcd71608f98af1e0ec19d965099ceab Mon Sep 17 00:00:00 2001
> > From: Tristan Partin <tristan@neon.tech>
> > Date: Wed, 17 May 2023 09:40:02 -0500
> > Subject: [PATCH postgres v1 12/17] Make finding pkg-config(python3) more
> >  robust
> >
> > It is a possibility that the installation can't be found. Checking for
> > Python.h is redundant with what Meson does internally.
>
> That's not what I saw - we had cases where Python.h was missing, but the
> python dependency was found. It's possible that this is dependent on the
> meson version.

Eli corrected me on this. Please see version 2 of the patchset.
>
> > From 47394ffd113d4170e955bc033841cb7e18fd3ac4 Mon Sep 17 00:00:00 2001
> > From: Tristan Partin <tristan@neon.tech>
> > Date: Wed, 17 May 2023 09:44:49 -0500
> > Subject: [PATCH postgres v1 14/17] Reduce branching on Meson version
> >
> > This code had a branch depending on Meson version. Instead, we can just
> > move the system checks to the if statement. I believe this also keeps
> > selinux and systemd from being looked for on non-Linux systems when
> > using Meson < 0.59. Before they would be checked, but obviously fail.
>
> I like the current version better - depending on the meson version makes it
> easy to find what needs to be removed when we upgrade the meson minimum
> version.

I think the savings of not looking up selinux/systemd on non-Linux
systems is pretty big. Would you accept a change of something like:

if meson.version().version_compare('>=0.59')
  # do old stuff
else if host_system == 'linux'
  # do new stuff
endif

Otherwise, I am happy to remove the patch.

> > From 189d3ac3d5593ce3e475813e4830a29bb4e96f70 Mon Sep 17 00:00:00 2001
> > From: Tristan Partin <tristan@neon.tech>
> > Date: Wed, 17 May 2023 10:36:52 -0500
> > Subject: [PATCH postgres v1 16/17] Add Meson overrides
> >
> > Meson has the ability to do transparent overrides when projects are used
> > as subprojects. For instance, say I am building a Postgres extension. I
> > can define Postgres to be a subproject of my extension given the
> > following wrap file:
> >
> > [wrap-git]
> > url = https://git.postgresql.org/git/postgresql.git
> > revision = master
> > depth = 1
> >
> > [provide]
> > dependency_names = libpq
> >
> > Then in my extension (root project), I can have the following line
> > snippet:
> >
> > libpq = dependency('libpq')
> >
> > This will tell Meson to transparently compile libpq prior to it
> > compiling my extension (because I depend on libpq) if libpq isn't found
> > on the host system.
> >
> > I have also added overrides for the various public-facing exectuables.
> > Though I don't expect them to get much usage, might as well go ahead and
> > override them. They can be used by adding the following line to the
> > aforementioned wrap file:
>
> I think adding more boilerplate to all these places does have some cost. So
> I'm not really convinced this is worth doign.

Could you explain more about what costs you foresee? I thought this was
a pretty free change :). Most of the meson.build files seems to be
pretty much copy/pastes of each other, so if a new executable came
around, then someone would just get the override line for essentially
free, minus changing the binary name/executable name.

> > From 5ee13f09e4101904dbc9887bd4844eb5f1cb4fea Mon Sep 17 00:00:00 2001
> > From: Tristan Partin <tristan@neon.tech>
> > Date: Wed, 17 May 2023 10:54:53 -0500
> > Subject: [PATCH postgres v1 17/17] Remove Meson program options for specifying
> >  paths
> >
> > Meson has a built-in way to override paths without polluting project
> > build options called machine files.
>
> I think meson machine files are just about unusable. For one, you can't
> actually change any of the options without creating a new build dir. For
> another, it's not something that easily can be done on the commandline.
>
> So I really don't want to remove these options.

I felt like this would be the most controversial change. What could be
done in upstream Meson to make this a more enjoyable experience? I do
however disagree with the usability of machine files. Could you add a
little context about what you find unusable about them?

Happy to revert the change after continuing the discussion, of course.

--
Tristan Partin
Neon (https://neon.tech)

Hi,

On 2023-06-09 13:15:27 -0500, Tristan Partin wrote:
> On Fri Jun 9, 2023 at 11:41 AM CDT, Andres Freund wrote:
> > I like the current version better - depending on the meson version makes it
> > easy to find what needs to be removed when we upgrade the meson minimum
> > version.
> 
> I think the savings of not looking up selinux/systemd on non-Linux
> systems is pretty big. Would you accept a change of something like:

For selinux it's default disabled, so it doesn't make a practical
difference. Outside of linux newer versions of meson are more common IME, so
I'm not really worried about it for systemd.


> if meson.version().version_compare('>=0.59')
>   # do old stuff
> else if host_system == 'linux'
>   # do new stuff
> endif
> 
> Otherwise, I am happy to remove the patch.

Hm, I don't quite know how this would end up looking like.


> > > From 189d3ac3d5593ce3e475813e4830a29bb4e96f70 Mon Sep 17 00:00:00 2001
> > > From: Tristan Partin <tristan@neon.tech>
> > > Date: Wed, 17 May 2023 10:36:52 -0500
> > > Subject: [PATCH postgres v1 16/17] Add Meson overrides
> > > 
> > > Meson has the ability to do transparent overrides when projects are used
> > > as subprojects. For instance, say I am building a Postgres extension. I
> > > can define Postgres to be a subproject of my extension given the
> > > following wrap file:
> > > 
> > > [wrap-git]
> > > url = https://git.postgresql.org/git/postgresql.git
> > > revision = master
> > > depth = 1
> > > 
> > > [provide]
> > > dependency_names = libpq
> > > 
> > > Then in my extension (root project), I can have the following line
> > > snippet:
> > > 
> > > libpq = dependency('libpq')
> > > 
> > > This will tell Meson to transparently compile libpq prior to it
> > > compiling my extension (because I depend on libpq) if libpq isn't found
> > > on the host system.
> > > 
> > > I have also added overrides for the various public-facing exectuables.
> > > Though I don't expect them to get much usage, might as well go ahead and
> > > override them. They can be used by adding the following line to the
> > > aforementioned wrap file:
> >
> > I think adding more boilerplate to all these places does have some cost. So
> > I'm not really convinced this is worth doign.
> 
> Could you explain more about what costs you foresee?

Repetitive code that needs to be added to each further binary we add. I don't
mind doing that if it has a use case, but I'm not sure I see the use case for
random binaries...


> > > From 5ee13f09e4101904dbc9887bd4844eb5f1cb4fea Mon Sep 17 00:00:00 2001
> > > From: Tristan Partin <tristan@neon.tech>
> > > Date: Wed, 17 May 2023 10:54:53 -0500
> > > Subject: [PATCH postgres v1 17/17] Remove Meson program options for specifying
> > >  paths
> > > 
> > > Meson has a built-in way to override paths without polluting project
> > > build options called machine files.
> >
> > I think meson machine files are just about unusable. For one, you can't
> > actually change any of the options without creating a new build dir. For
> > another, it's not something that easily can be done on the commandline.
> >
> > So I really don't want to remove these options.
> 
> I felt like this would be the most controversial change. What could be
> done in upstream Meson to make this a more enjoyable experience?

I think *requiring* separate files is a really poor experience when you come
from some other system, where those could trivially be overwritten on the
commandline.

The biggest change to make them more usable would be to properly reconfigure
when the contents of machine file change. IIRC configure is rerun, but the
changes aren't taken into account.


> I do however disagree with the usability of machine files. Could you add a
> little context about what you find unusable about them?

I can quickly change a meson option and run a build and tests. Trivially
scriptable. Whereas with a machine file I need to write code to edit a machine
file, re-configure from scratch, and only then can build / run tests.

It's particularly bad for cross builds, where unfortunately cross files can't
be avoided. It's imo the one area where autoconf beats meson handily.
--host=x86_64-w64-mingw32 works for autoconf. Whereas for meson I need to
manually write a cross file with a bunch of binaries set.

https://github.com/anarazel/postgres/commit/ae53f21d06b4dadf8e6b90df84000fad9a769eaf#diff-3420ebab4f1dbe2ba7102565b0b84e4d6d01fb8b3c1e375bd439eed604e743f8R1

There's some helpers aiming to generate cross files, but I've not been able to
generate something useful for cross compiling to windows, for example.

I've been unable to generate something like referenced in the above commit in
a reasonably concise way with env2mfile.


In the end, I also just don't see a meaningful benefit in forcing the use of
machine files.

Greetings,

Andres Freund

On Fri Jun 9, 2023 at 1:36 PM CDT, Andres Freund wrote:
> Hi,
>
> On 2023-06-09 13:15:27 -0500, Tristan Partin wrote:
> > On Fri Jun 9, 2023 at 11:41 AM CDT, Andres Freund wrote:
> > > I like the current version better - depending on the meson version makes it
> > > easy to find what needs to be removed when we upgrade the meson minimum
> > > version.
> >
> > I think the savings of not looking up selinux/systemd on non-Linux
> > systems is pretty big. Would you accept a change of something like:
>
> For selinux it's default disabled, so it doesn't make a practical
> difference. Outside of linux newer versions of meson are more common IME, so
> I'm not really worried about it for systemd.
>
>
> > if meson.version().version_compare('>=0.59')
> >   # do old stuff
> > else if host_system == 'linux'
> >   # do new stuff
> > endif
> >
> > Otherwise, I am happy to remove the patch.
>
> Hm, I don't quite know how this would end up looking like.

Actually, nevermind. I don't know what I was talking about. I will just
go ahead and remove this patch from the set.

> > > > From 189d3ac3d5593ce3e475813e4830a29bb4e96f70 Mon Sep 17 00:00:00 2001
> > > > From: Tristan Partin <tristan@neon.tech>
> > > > Date: Wed, 17 May 2023 10:36:52 -0500
> > > > Subject: [PATCH postgres v1 16/17] Add Meson overrides
> > > >
> > > > Meson has the ability to do transparent overrides when projects are used
> > > > as subprojects. For instance, say I am building a Postgres extension. I
> > > > can define Postgres to be a subproject of my extension given the
> > > > following wrap file:
> > > >
> > > > [wrap-git]
> > > > url = https://git.postgresql.org/git/postgresql.git
> > > > revision = master
> > > > depth = 1
> > > >
> > > > [provide]
> > > > dependency_names = libpq
> > > >
> > > > Then in my extension (root project), I can have the following line
> > > > snippet:
> > > >
> > > > libpq = dependency('libpq')
> > > >
> > > > This will tell Meson to transparently compile libpq prior to it
> > > > compiling my extension (because I depend on libpq) if libpq isn't found
> > > > on the host system.
> > > >
> > > > I have also added overrides for the various public-facing exectuables.
> > > > Though I don't expect them to get much usage, might as well go ahead and
> > > > override them. They can be used by adding the following line to the
> > > > aforementioned wrap file:
> > >
> > > I think adding more boilerplate to all these places does have some cost. So
> > > I'm not really convinced this is worth doign.
> >
> > Could you explain more about what costs you foresee?
>
> Repetitive code that needs to be added to each further binary we add. I don't
> mind doing that if it has a use case, but I'm not sure I see the use case for
> random binaries...

I want to make sure I am only adding it to things that are user-facing.
So if I have added the line to executables that are for internal use
only, please correct me. In general, I override for all user-facing
programs/dependencies because I never know how some end-user might use
the binaries.

Perhaps what might sway you is that the old method (libpq = libpq_dep)
is very error prone because variable names become part of the public API
of your build description which no one likes. In the new way, which is
only possible when specifying overrides, as long as binary names or
pkg-config names don't change, you get stability no matter how you name
your variables.

> > > > From 5ee13f09e4101904dbc9887bd4844eb5f1cb4fea Mon Sep 17 00:00:00 2001
> > > > From: Tristan Partin <tristan@neon.tech>
> > > > Date: Wed, 17 May 2023 10:54:53 -0500
> > > > Subject: [PATCH postgres v1 17/17] Remove Meson program options for specifying
> > > >  paths
> > > >
> > > > Meson has a built-in way to override paths without polluting project
> > > > build options called machine files.
> > >
> > > I think meson machine files are just about unusable. For one, you can't
> > > actually change any of the options without creating a new build dir. For
> > > another, it's not something that easily can be done on the commandline.
> > >
> > > So I really don't want to remove these options.
> >
> > I felt like this would be the most controversial change. What could be
> > done in upstream Meson to make this a more enjoyable experience?
>
> I think *requiring* separate files is a really poor experience when you come
> from some other system, where those could trivially be overwritten on the
> commandline.

Hmm. I could maybe ask around for a --program command line option. Could
you provide the syntax for what autotools does? That way I can come to
the discussion in the Meson channel with prior art. I personally don't
find the files that annoying, but to each their own.

> The biggest change to make them more usable would be to properly reconfigure
> when the contents of machine file change. IIRC configure is rerun, but the
> changes aren't taken into account.

I could not reproduce this. Perhaps you were testing with an older Meson
where that was the case

# meson.build
project('mytest')

myprog = find_program('myprog')
message(myprog.full_path())

test('dummy', find_program('echo'), args: [myprog.full_path()])

# file.ini
[binaries]
myprog = '/usr/bin/python3'

# CLI
meson setup build
meson test -C build
sed -i 's/python3/python2/' file.ini
meson test -C build

> > I do however disagree with the usability of machine files. Could you add a
> > little context about what you find unusable about them?
>
> I can quickly change a meson option and run a build and tests. Trivially
> scriptable. Whereas with a machine file I need to write code to edit a machine
> file, re-configure from scratch, and only then can build / run tests.
>
> It's particularly bad for cross builds, where unfortunately cross files can't
> be avoided. It's imo the one area where autoconf beats meson handily.
> --host=x86_64-w64-mingw32 works for autoconf. Whereas for meson I need to
> manually write a cross file with a bunch of binaries set.
>
https://github.com/anarazel/postgres/commit/ae53f21d06b4dadf8e6b90df84000fad9a769eaf#diff-3420ebab4f1dbe2ba7102565b0b84e4d6d01fb8b3c1e375bd439eed604e743f8R1

One thing that would help you with cross files is to use constants[0].

> There's some helpers aiming to generate cross files, but I've not been able to
> generate something useful for cross compiling to windows, for example.
>
> I've been unable to generate something like referenced in the above commit in
> a reasonably concise way with env2mfile.

I, too, could not generate anything meaningful with the following
command line.

CC=testcc CXX=testcxx AR=testar STRIP=teststrip \
    PKGCONFIG=testpkgconfig WINDRES=testwindres meson env2mfile \
    --cross --system windows --cpu x86_64 --cpu-family x86_64 \
    --endian little -o windows.ini

[binaries]
# Compilers
c = ['testcc']
cpp = ['testcxx']

# Other binaries

[properties]

[host_machine]
cpu = 'x86_64'
cpu_family = 'x86_64'
endian = 'little'
system = 'windows'

> In the end, I also just don't see a meaningful benefit in forcing the use of
> machine files.

I think it is best to use patterns tools want you to use. If aren't moved at
all by the reconfigure behavior actually working, I will drop the patch.

[0]: https://mesonbuild.com/Machine-files.html#constants

--
Tristan Partin
Neon (https://neon.tech)

Hi,

On 2023-06-12 11:54:42 -0500, Tristan Partin wrote:
> On Fri Jun 9, 2023 at 1:36 PM CDT, Andres Freund wrote:
> > The biggest change to make them more usable would be to properly reconfigure
> > when the contents of machine file change. IIRC configure is rerun, but the
> > changes aren't taken into account.
> 
> I could not reproduce this. Perhaps you were testing with an older Meson
> where that was the case
> 
> # meson.build
> project('mytest')
> 
> myprog = find_program('myprog')
> message(myprog.full_path())
> 
> test('dummy', find_program('echo'), args: [myprog.full_path()])
> 
> # file.ini
> [binaries]
> myprog = '/usr/bin/python3'
> 
> # CLI
> meson setup build
> meson test -C build
> sed -i 's/python3/python2/' file.ini
> meson test -C build

It's possible that it doesn't happen in all contexts. I just reproduced the
problem I had, changing

[binaries]
llvm-config = '/usr/bin/llvm-config-13'

to

[binaries]
llvm-config = '/usr/bin/llvm-config-14'

does not change which version is used in an existing build tree, but does
change what's used in a new build tree.

Same with e.g. changing the C compiler version in a machine file. That also
only takes effect in a new tree.


This is with meson HEAD, updated earlier today.


> > In the end, I also just don't see a meaningful benefit in forcing the use of
> > machine files.
> 
> I think it is best to use patterns tools want you to use.

Sometimes. I'd perhaps have a different view if we weren't migrating from
autoconf, where overwriting binaries was trivially possible...

Greetings,

Andres Freund

On Mon Jun 12, 2023 at 12:20 PM CDT, Andres Freund wrote:
> Hi,
>
> On 2023-06-12 11:54:42 -0500, Tristan Partin wrote:
> > On Fri Jun 9, 2023 at 1:36 PM CDT, Andres Freund wrote:
> > > The biggest change to make them more usable would be to properly reconfigure
> > > when the contents of machine file change. IIRC configure is rerun, but the
> > > changes aren't taken into account.
> >
> > I could not reproduce this. Perhaps you were testing with an older Meson
> > where that was the case
> >
> > # meson.build
> > project('mytest')
> >
> > myprog = find_program('myprog')
> > message(myprog.full_path())
> >
> > test('dummy', find_program('echo'), args: [myprog.full_path()])
> >
> > # file.ini
> > [binaries]
> > myprog = '/usr/bin/python3'
> >
> > # CLI
> > meson setup build
> > meson test -C build
> > sed -i 's/python3/python2/' file.ini
> > meson test -C build
>
> It's possible that it doesn't happen in all contexts. I just reproduced the
> problem I had, changing
>
> [binaries]
> llvm-config = '/usr/bin/llvm-config-13'
>
> to
>
> [binaries]
> llvm-config = '/usr/bin/llvm-config-14'
>
> does not change which version is used in an existing build tree, but does
> change what's used in a new build tree.
>
> Same with e.g. changing the C compiler version in a machine file. That also
> only takes effect in a new tree.
>
>
> This is with meson HEAD, updated earlier today.

Mind opening a Meson issue if one doesn't exist already?

> > > In the end, I also just don't see a meaningful benefit in forcing the use of
> > > machine files.
> >
> > I think it is best to use patterns tools want you to use.
>
> Sometimes. I'd perhaps have a different view if we weren't migrating from
> autoconf, where overwriting binaries was trivially possible...

I'll see what I can advocate for, regardless. The following things seem
relevant. Might be useful to track in your meta issue on your fork.

https://github.com/mesonbuild/meson/issues/7755
https://github.com/mesonbuild/meson/pull/11561
https://github.com/mesonbuild/meson/issues/6180
https://github.com/mesonbuild/meson/issues/11294

Attached you will find a v3 with the offending commits removed. I did
leave the overrides in since you didn't mention it in your last email.

--
Tristan Partin
Neon (https://neon.tech)

On 12.06.23 20:48, Tristan Partin wrote:
> Attached you will find a v3 with the offending commits removed. I did
> leave the overrides in since you didn't mention it in your last email.

Patches 1-14 look ok to me.  (But I didn't test anything, so some 
caveats about whether the non-cosmetic patches actually work apply.)  If 
we're fine-tuning the capitalization the options descriptions, let's 
also turn "tap tests" into "TAP tests" and "TCL" into "Tcl".

Patch 15 about the wrap integration, I'm not sure.  I share the concerns 
about whether this is worth maintaining.  Maybe put this patch into the 
commitfest separately, to allow for further study and testing.  (The 
other patches, if they are acceptable, ought to go into PG16, I think.)

On Mon Jun 12, 2023 at 4:24 PM CDT, Peter Eisentraut wrote:
> On 12.06.23 20:48, Tristan Partin wrote:
> > Attached you will find a v3 with the offending commits removed. I did
> > leave the overrides in since you didn't mention it in your last email.
>
> Patches 1-14 look ok to me.  (But I didn't test anything, so some
> caveats about whether the non-cosmetic patches actually work apply.)  If
> we're fine-tuning the capitalization the options descriptions, let's
> also turn "tap tests" into "TAP tests" and "TCL" into "Tcl".

I'll get to that.

> Patch 15 about the wrap integration, I'm not sure.  I share the concerns
> about whether this is worth maintaining.  Maybe put this patch into the
> commitfest separately, to allow for further study and testing.  (The
> other patches, if they are acceptable, ought to go into PG16, I think.)

Ok. I will split it off for further discussion. Expect a v4 tomorrow
with a few extra changes with regard to another review from a Meson
maintainer.

--
Tristan Partin
Neon (https://neon.tech)

On Fri Jun 9, 2023 at 1:36 PM CDT, Andres Freund wrote:
> On 2023-06-09 13:15:27 -0500, Tristan Partin wrote:
> > On Fri Jun 9, 2023 at 11:41 AM CDT, Andres Freund wrote:
> > > > From 189d3ac3d5593ce3e475813e4830a29bb4e96f70 Mon Sep 17 00:00:00 2001
> > > > From: Tristan Partin <tristan@neon.tech>
> > > > Date: Wed, 17 May 2023 10:36:52 -0500
> > > > Subject: [PATCH postgres v1 16/17] Add Meson overrides
> > > >
> > > > Meson has the ability to do transparent overrides when projects are used
> > > > as subprojects. For instance, say I am building a Postgres extension. I
> > > > can define Postgres to be a subproject of my extension given the
> > > > following wrap file:
> > > >
> > > > [wrap-git]
> > > > url = https://git.postgresql.org/git/postgresql.git
> > > > revision = master
> > > > depth = 1
> > > >
> > > > [provide]
> > > > dependency_names = libpq
> > > >
> > > > Then in my extension (root project), I can have the following line
> > > > snippet:
> > > >
> > > > libpq = dependency('libpq')
> > > >
> > > > This will tell Meson to transparently compile libpq prior to it
> > > > compiling my extension (because I depend on libpq) if libpq isn't found
> > > > on the host system.
> > > >
> > > > I have also added overrides for the various public-facing exectuables.
> > > > Though I don't expect them to get much usage, might as well go ahead and
> > > > override them. They can be used by adding the following line to the
> > > > aforementioned wrap file:
> > >
> > > I think adding more boilerplate to all these places does have some cost. So
> > > I'm not really convinced this is worth doign.
> >
> > Could you explain more about what costs you foresee?
>
> Repetitive code that needs to be added to each further binary we add. I don't
> mind doing that if it has a use case, but I'm not sure I see the use case for
> random binaries...

I was thinking today. When you initially wrote the build, did you try
using the src/bin/meson.build file as the place where all the binaries
were built? As you say, most of the src/bin/xxx/meson.build files are
extrememly reptitive.

We had a similar-ish issue in my last project which I solved like:

https://github.com/hse-project/hse/blob/master/tools/meson.build#L20-L405

This is a pattern I used quite frequently in that project. One benefit
of this approach is that the binaries all end up next to each other in
the build tree which is eventually how they'll be laid out in the
install destination. The other benefit is of course reducing reptitive
code.

- ./build/src/bin/psql/psql
+ ./build/src/bin/psql

Let me know what you think.

--
Tristan Partin
Neon (https://neon.tech)

On Mon Jun 12, 2023 at 4:43 PM CDT, Tristan Partin wrote:
> On Mon Jun 12, 2023 at 4:24 PM CDT, Peter Eisentraut wrote:
> > On 12.06.23 20:48, Tristan Partin wrote:
> > > Attached you will find a v3 with the offending commits removed. I did
> > > leave the overrides in since you didn't mention it in your last email.
> >
> > Patches 1-14 look ok to me.  (But I didn't test anything, so some
> > caveats about whether the non-cosmetic patches actually work apply.)  If
> > we're fine-tuning the capitalization the options descriptions, let's
> > also turn "tap tests" into "TAP tests" and "TCL" into "Tcl".
>
> I'll get to that.

Done.

> > Patch 15 about the wrap integration, I'm not sure.  I share the concerns
> > about whether this is worth maintaining.  Maybe put this patch into the
> > commitfest separately, to allow for further study and testing.  (The
> > other patches, if they are acceptable, ought to go into PG16, I think.)
>
> Ok. I will split it off for further discussion. Expect a v4 tomorrow
> with a few extra changes with regard to another review from a Meson
> maintainer.

Attached. I did have an idea to help with repetitive build descriptions
in:
https://www.postgresql.org/message-id/CTBSCT2V1TVP.2AUJVJLNWQVG3%40gonk.
I kind of want to see Andres' response, before this gets merged. But
that could also be a follow on commit if he likes the idea. I'll leave
it up to you to decide.

--
Tristan Partin
Neon (https://neon.tech)

Wow. I didn't attach them...

--
Tristan Partin
Neon (https://neon.tech)

Forgot that I had gotten a review from a Meson maintainer. The last two
patches in this set are new. One is just a simple spelling correction.

--
Tristan Partin
Neon (https://neon.tech)

On 13.06.23 22:47, Tristan Partin wrote:
> Forgot that I had gotten a review from a Meson maintainer. The last two
> patches in this set are new. One is just a simple spelling correction.

I have committed patches 0001-0006, 0008, 0010, 0013, 0014, 0016, which 
are all pretty much cosmetic.

The following patches are now still pending further review:

v5-0007-Tie-adding-C-support-to-the-llvm-Meson-option.patch
v5-0009-Remove-return-code-check.patch
v5-0011-Pass-feature-option-through-to-required-kwarg.patch
v5-0012-Make-finding-pkg-config-python3-more-robust.patch
v5-0015-Clean-up-some-usage-of-Meson-features.patch

On Thu Jun 29, 2023 at 6:18 AM CDT, Peter Eisentraut wrote:
> On 13.06.23 22:47, Tristan Partin wrote:
> > Forgot that I had gotten a review from a Meson maintainer. The last two
> > patches in this set are new. One is just a simple spelling correction.
>
> I have committed patches 0001-0006, 0008, 0010, 0013, 0014, 0016, which
> are all pretty much cosmetic.

Thanks Peter.

--
Tristan Partin
Neon (https://neon.tech)

Hi,

On 2023-06-29 13:18:21 +0200, Peter Eisentraut wrote:
> On 13.06.23 22:47, Tristan Partin wrote:
> > Forgot that I had gotten a review from a Meson maintainer. The last two
> > patches in this set are new. One is just a simple spelling correction.
>
> I have committed patches 0001-0006, 0008, 0010, 0013, 0014, 0016, which are
> all pretty much cosmetic.

Thanks Peter, Tristan!  I largely couldn't muster an opinion on most of
these...


> The following patches are now still pending further review:
>
> v5-0007-Tie-adding-C-support-to-the-llvm-Meson-option.patch

Hm. One minor disadvantage of this is that if no c++ compiler was found, you
can't really see anything about llvm in the the output, nor in meson-log.txt,
making it somewhat hard to figure out why llvm was disabled.

I think something like

elif llvmopt.auto()
  message('llvm requires a C++ compiler')
endif

Should do the trick?


> v5-0009-Remove-return-code-check.patch

Pushed.


> v5-0011-Pass-feature-option-through-to-required-kwarg.patch

I'm a bit confused how it ended how it's looking like it is right now, but
... :)

I'm thinking of merging 0011 and relevant parts of 0012 and 0015 into one.


> v5-0012-Make-finding-pkg-config-python3-more-robust.patch

The commit message here is clearly outdated (still talking about Python.h
check not being required).  Does the remainder actually add any robustness?

I'm on board with removing unnecessary .enabled(), but from what I understand
we don't gain anything from adding the if python3_inst.found() branch?


> v5-0015-Clean-up-some-usage-of-Meson-features.patch

For me that's not really an improvement in legibility, the indentation for the
bulk of each test helps parse things visually.  In some cases the removed "if
feature.disabled()" actually leads to tests being executed when a feature is
disabled, e.g. perl -MConfig ... would now be run even perl is disabled.


Attached my version of 0007 and 0011 (with some changes from 0012 and
0015). I'm running a test of those with the extended CI I have in a branch...

Greetings,

Andres Freund

On Thu Jun 29, 2023 at 12:35 PM CDT, Andres Freund wrote:
> Hi,
>
> On 2023-06-29 13:18:21 +0200, Peter Eisentraut wrote:
> > On 13.06.23 22:47, Tristan Partin wrote:
> > > Forgot that I had gotten a review from a Meson maintainer. The last two
> > > patches in this set are new. One is just a simple spelling correction.
> >
> > I have committed patches 0001-0006, 0008, 0010, 0013, 0014, 0016, which are
> > all pretty much cosmetic.
>
> Thanks Peter, Tristan!  I largely couldn't muster an opinion on most of
> these...
>
>
> > The following patches are now still pending further review:
> >
> > v5-0007-Tie-adding-C-support-to-the-llvm-Meson-option.patch
>
> Hm. One minor disadvantage of this is that if no c++ compiler was found, you
> can't really see anything about llvm in the the output, nor in meson-log.txt,
> making it somewhat hard to figure out why llvm was disabled.
>
> I think something like
>
> elif llvmopt.auto()
>   message('llvm requires a C++ compiler')
> endif
>
> Should do the trick?

Your patch looks great to me.

> > v5-0011-Pass-feature-option-through-to-required-kwarg.patch
>
> I'm a bit confused how it ended how it's looking like it is right now, but
> ... :)
>
> I'm thinking of merging 0011 and relevant parts of 0012 and 0015 into one.

Your patch looks great to me.

> > v5-0012-Make-finding-pkg-config-python3-more-robust.patch
>
> The commit message here is clearly outdated (still talking about Python.h
> check not being required).  Does the remainder actually add any robustness?
>
> I'm on board with removing unnecessary .enabled(), but from what I understand
> we don't gain anything from adding the if python3_inst.found() branch?

Attached is a more up to date patch, which removes the old part of the
commit message. I guess robust is in the eye of the beholder. It is
definitely possible for the installation to not be found (Python.h not
existing in newer versions of Meson as an example). All my patch would
do is keep the build from crashing if and only if the installation
wasn't found.

The unnecessary .enabled() could be folded into the other patch if you
so chose.

> > v5-0015-Clean-up-some-usage-of-Meson-features.patch
>
> For me that's not really an improvement in legibility, the indentation for the
> bulk of each test helps parse things visually.  In some cases the removed "if
> feature.disabled()" actually leads to tests being executed when a feature is
> disabled, e.g. perl -MConfig ... would now be run even perl is disabled.

Makes sense to not take the patch then.

> Attached my version of 0007 and 0011 (with some changes from 0012 and
> 0015). I'm running a test of those with the extended CI I have in a branch...

Thanks for the further review. Did you by chance see my other email in
another branch of this thread[0]?

[0]: https://www.postgresql.org/message-id/CTBSCT2V1TVP.2AUJVJLNWQVG3@gonk

--
Tristan Partin
Neon (https://neon.tech)

Hi,

On 2023-06-29 13:34:42 -0500, Tristan Partin wrote:
> On Thu Jun 29, 2023 at 12:35 PM CDT, Andres Freund wrote:
> > > v5-0012-Make-finding-pkg-config-python3-more-robust.patch
> >
> > The commit message here is clearly outdated (still talking about Python.h
> > check not being required).  Does the remainder actually add any robustness?
> >
> > I'm on board with removing unnecessary .enabled(), but from what I understand
> > we don't gain anything from adding the if python3_inst.found() branch?
> 
> Attached is a more up to date patch, which removes the old part of the
> commit message. I guess robust is in the eye of the beholder. It is
> definitely possible for the installation to not be found (Python.h not
> existing in newer versions of Meson as an example). All my patch would
> do is keep the build from crashing if and only if the installation
> wasn't found.

Ah - I somehow thought .find_installation().dependency() would return a
not-found dependency when the install wasn't located.


> > Attached my version of 0007 and 0011 (with some changes from 0012 and
> > 0015). I'm running a test of those with the extended CI I have in a branch...
> 
> Thanks for the further review. Did you by chance see my other email in
> another branch of this thread[0]?
> 
> [0]: https://www.postgresql.org/message-id/CTBSCT2V1TVP.2AUJVJLNWQVG3@gonk

I had planned to, but somehow forgot. Will reply.

Greetings,

Andres Freund

Hi,

On 2023-06-13 14:56:36 -0500, Tristan Partin wrote:
> I was thinking today. When you initially wrote the build, did you try
> using the src/bin/meson.build file as the place where all the binaries
> were built? As you say, most of the src/bin/xxx/meson.build files are
> extrememly reptitive.

> We had a similar-ish issue in my last project which I solved like:
> 
> https://github.com/hse-project/hse/blob/master/tools/meson.build#L20-L405
> 
> This is a pattern I used quite frequently in that project. One benefit
> of this approach is that the binaries all end up next to each other in
> the build tree which is eventually how they'll be laid out in the
> install destination. The other benefit is of course reducing reptitive
> code.

I think the build directory and the source code directory not matching in
structure would have made it considerably harder sell for people to migrate.

I.e. I considered it, but due to meson's "no outputs outside of the current
directory" rule, it didn't (and sadly still doesn't) really seem viable.

Greetings,

Andres Freund

On Thu Jun 29, 2023 at 1:58 PM CDT, Andres Freund wrote:
> Hi,
>
> On 2023-06-29 13:34:42 -0500, Tristan Partin wrote:
> > On Thu Jun 29, 2023 at 12:35 PM CDT, Andres Freund wrote:
> > > > v5-0012-Make-finding-pkg-config-python3-more-robust.patch
> > >
> > > The commit message here is clearly outdated (still talking about Python.h
> > > check not being required).  Does the remainder actually add any robustness?
> > >
> > > I'm on board with removing unnecessary .enabled(), but from what I understand
> > > we don't gain anything from adding the if python3_inst.found() branch?
> >
> > Attached is a more up to date patch, which removes the old part of the
> > commit message. I guess robust is in the eye of the beholder. It is
> > definitely possible for the installation to not be found (Python.h not
> > existing in newer versions of Meson as an example). All my patch would
> > do is keep the build from crashing if and only if the installation
> > wasn't found.
>
> Ah - I somehow thought .find_installation().dependency() would return a
> not-found dependency when the install wasn't located.

Inspecting the Meson source code... If find_installation() fails, you
get returned a NonExistingExternalProgram, which doesn't implement
dependency()[0].

> > > Attached my version of 0007 and 0011 (with some changes from 0012 and
> > > 0015). I'm running a test of those with the extended CI I have in a branch...
> >
> > Thanks for the further review. Did you by chance see my other email in
> > another branch of this thread[0]?
> >
> > [0]: https://www.postgresql.org/message-id/CTBSCT2V1TVP.2AUJVJLNWQVG3@gonk
>
> I had planned to, but somehow forgot. Will reply.

Thanks!

[0]: https://github.com/mesonbuild/meson/blob/master/mesonbuild/programs.py#L333

--
Tristan Partin
Neon (https://neon.tech)

On Thu Jun 29, 2023 at 2:02 PM CDT, Andres Freund wrote:
> Hi,
>
> On 2023-06-13 14:56:36 -0500, Tristan Partin wrote:
> > I was thinking today. When you initially wrote the build, did you try
> > using the src/bin/meson.build file as the place where all the binaries
> > were built? As you say, most of the src/bin/xxx/meson.build files are
> > extrememly reptitive.
>
> > We had a similar-ish issue in my last project which I solved like:
> >
> > https://github.com/hse-project/hse/blob/master/tools/meson.build#L20-L405
> >
> > This is a pattern I used quite frequently in that project. One benefit
> > of this approach is that the binaries all end up next to each other in
> > the build tree which is eventually how they'll be laid out in the
> > install destination. The other benefit is of course reducing reptitive
> > code.
>
> I think the build directory and the source code directory not matching in
> structure would have made it considerably harder sell for people to migrate.
>
> I.e. I considered it, but due to meson's "no outputs outside of the current
> directory" rule, it didn't (and sadly still doesn't) really seem viable.

Yeah, I guess it is a matter if you like the layout being closer to the
installation or the source tree at the expense of repetition. I am
partial to the installation since it is less to type if you run a binary
from the build directory and less repetition, but all good. Maybe
something that could be reconsidered when autotools is dropped.

I still think the overrides are important, at the very least for libpq,
but I will defer to your aforementioned decision for now.

--
Tristan Partin
Neon (https://neon.tech)

Hi,

On 2023-06-29 14:07:19 -0500, Tristan Partin wrote:
> I still think the overrides are important, at the very least for libpq,
> but I will defer to your aforementioned decision for now.

libpq makes sense to me, fwiw. Just doing it for all binaries individually
didn't seem as obviously beneficial.

FWIW, it seems it could be handled somewhat centrally for binaries, the
bin_targets array should have all that's needed?

Some things won't work from the build directory, btw. E.g. initdb or postgres
itself.

Greetings,

Andres Freund

On Thu Jun 29, 2023 at 2:13 PM CDT, Andres Freund wrote:
> Hi,
>
> On 2023-06-29 14:07:19 -0500, Tristan Partin wrote:
> > I still think the overrides are important, at the very least for libpq,
> > but I will defer to your aforementioned decision for now.
>
> libpq makes sense to me, fwiw. Just doing it for all binaries individually
> didn't seem as obviously beneficial.
>
> FWIW, it seems it could be handled somewhat centrally for binaries, the
> bin_targets array should have all that's needed?

I will send a follow-up patch with at least libpq overridden. I will
investigate the bin_targets.

> Some things won't work from the build directory, btw. E.g. initdb or postgres
> itself.

I have already fallen victim to this! Lesson learned :).

--
Tristan Partin
Neon (https://neon.tech)

On Thu Jun 29, 2023 at 2:17 PM CDT, Tristan Partin wrote:
> On Thu Jun 29, 2023 at 2:13 PM CDT, Andres Freund wrote:
> > Hi,
> >
> > On 2023-06-29 14:07:19 -0500, Tristan Partin wrote:
> > > I still think the overrides are important, at the very least for libpq,
> > > but I will defer to your aforementioned decision for now.
> >
> > libpq makes sense to me, fwiw. Just doing it for all binaries individually
> > didn't seem as obviously beneficial.
> >
> > FWIW, it seems it could be handled somewhat centrally for binaries, the
> > bin_targets array should have all that's needed?
>
> I will send a follow-up patch with at least libpq overridden. I will
> investigate the bin_targets.

Attached is a patch which does just that without overriding the
binaries. I investigated the bin_targets stuff, but some test
executables get added there, so it wouldn't work out that well.

--
Tristan Partin
Neon (https://neon.tech)

On 2023-Jul-12, Tristan Partin wrote:

> Attached is a patch which does just that without overriding the
> binaries. I investigated the bin_targets stuff, but some test
> executables get added there, so it wouldn't work out that well.

This seems useful.  Maybe we should have some documentation changes to
go with it, because otherwise it seems a bit too obscure.

Maybe there are other subdirs where this would be useful.  ecpg maybe?
(Much less widely used, but if it's this simple, it shouldn't be much of
a burden)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On Wed Jul 12, 2023 at 11:39 AM CDT, Alvaro Herrera wrote:
> On 2023-Jul-12, Tristan Partin wrote:
>
> > Attached is a patch which does just that without overriding the
> > binaries. I investigated the bin_targets stuff, but some test
> > executables get added there, so it wouldn't work out that well.
>
> This seems useful.  Maybe we should have some documentation changes to
> go with it, because otherwise it seems a bit too obscure.

Do you have a place in mind on where to document it?

> Maybe there are other subdirs where this would be useful.  ecpg maybe?
> (Much less widely used, but if it's this simple, it shouldn't be much of
> a burden)

A previous version of this patch[0] did it to all public facing
binaries. Andres wasn't super interested in that.

[0]: https://www.postgresql.org/message-id/CSPIJVUDZFKX.3KHMOAVGF94RV%40c3po

--
Tristan Partin
Neon (https://neon.tech)

Hi,

On 2023-06-29 13:34:42 -0500, Tristan Partin wrote:
> On Thu Jun 29, 2023 at 12:35 PM CDT, Andres Freund wrote:
> > Hm. One minor disadvantage of this is that if no c++ compiler was found, you
> > can't really see anything about llvm in the the output, nor in meson-log.txt,
> > making it somewhat hard to figure out why llvm was disabled.
> >
> > I think something like
> >
> > elif llvmopt.auto()
> >   message('llvm requires a C++ compiler')
> > endif
> >
> > Should do the trick?
> 
> Your patch looks great to me.
> 
> > > v5-0011-Pass-feature-option-through-to-required-kwarg.patch
> >
> > I'm a bit confused how it ended how it's looking like it is right now, but
> > ... :)
> >
> > I'm thinking of merging 0011 and relevant parts of 0012 and 0015 into one.
> 
> Your patch looks great to me.

Pushed these. Thanks!

Greetings,

Andres Freund

Did you need anything more from the "Make finding pkg-config(python3)
more robust" patch? That one doesn't seem to have been applied yet.

Thanks for your reviews thus far.

--
Tristan Partin
Neon (https://neon.tech)

Hi,

On 2023-07-12 18:53:03 -0500, Tristan Partin wrote:
> Did you need anything more from the "Make finding pkg-config(python3)
> more robust" patch? That one doesn't seem to have been applied yet.

Sorry, was overloaded at the time and then lost track of it. Pushed now!

Greetings,

Andres Freund