Meson build updates

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]https://mesonbuild.com/Machine-files.html#constants.

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:
/messages/by-id/CTBSCT2V1TVP.2AUJVJLNWQVG3@gonk.
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]/messages/by-id/CTBSCT2V1TVP.2AUJVJLNWQVG3@gonk?

[0]: /messages/by-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]: /messages/by-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