Content-Type: multipart/mixed;
 boundary=2eca9e845ddb461adaf774511162360fbe1dac129c6f3f6dc37a19059948
Mime-Version: 1.0
Date: Thu, 29 Jun 2023 13:34:42 -0500
Cc: <pgsql-hackers@postgresql.org>
Subject: Re: Meson build updates
To: "Andres Freund" <andres@anarazel.de>, "Peter Eisentraut"
 <peter@eisentraut.org>
From: "Tristan Partin" <tristan@neon.tech>
Message-Id: <CTPCMQ52DU6N.RYKQDQ2ZQ1YQ@gonk>
References: <20230609183603.o56z3ngtak6qhb3s@awork3.anarazel.de>
 <CTATUZLNWYKO.2PMYWTUA85JUK@gonk>
 <20230612172038.f2xf2cwlnhjldqng@awork3.anarazel.de>
 <CTAW8GBWEJ7F.129Y59WDVDNMW@gonk>
 <277c356e-7ef7-4c87-f4b2-97a13914dd02@eisentraut.org>
 <CTB00342PAX7.36RK20RD57EUH@gonk> <CTBSP2GSV4IO.2J7QXVJBTI359@gonk>
 <CTBSPHEFKSTA.3H4L369UPLIRZ@gonk> <CTBTF7D3NT2N.2KVYEWW9NCHY0@gonk>
 <fc86a216-4827-be14-2468-7309ea189f67@eisentraut.org>
 <20230629173532.cdcydvqf3x5j7524@awork3.anarazel.de>
In-Reply-To: <20230629173532.cdcydvqf3x5j7524@awork3.anarazel.de>
Archived-At: 
 <https://www.postgresql.org/message-id/CTPCMQ52DU6N.RYKQDQ2ZQ1YQ%40gonk>
Precedence: bulk

--2eca9e845ddb461adaf774511162360fbe1dac129c6f3f6dc37a19059948
Content-Type: multipart/alternative;
 boundary=f7137547e7babf4fbcba352a1eed9da70ec8aa73a68c2bc5f4cb7a7d0bc7

--f7137547e7babf4fbcba352a1eed9da70ec8aa73a68c2bc5f4cb7a7d0bc7
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline
Content-Type: text/plain; charset=UTF-8

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 t=
wo
> > > 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, bu=
t
> ... :)
>
> 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 robustnes=
s?
>
> I'm on board with removing unnecessary .enabled(), but from what I unders=
tand
> 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 fo=
r 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 branc=
h...

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

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

--f7137547e7babf4fbcba352a1eed9da70ec8aa73a68c2bc5f4cb7a7d0bc7--

--2eca9e845ddb461adaf774511162360fbe1dac129c6f3f6dc37a19059948
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename=v6-0001-Make-finding-pkg-config-python3-more-robust.patch
Content-Type: text/x-patch; charset=utf-8;
 name=v6-0001-Make-finding-pkg-config-python3-more-robust.patch

RnJvbSA1YTFjMWMxNDgzZGNiOWM3ZTM0YTU0NjJkZGFhN2EzN2FkYmQ2Y2M3IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBUcmlzdGFuIFBhcnRpbiA8dHJpc3RhbkBuZW9uLnRlY2g+CkRh
dGU6IFdlZCwgMTcgTWF5IDIwMjMgMDk6NDA6MDIgLTA1MDAKU3ViamVjdDogW1BBVENIIHY2XSBN
YWtlIGZpbmRpbmcgcGtnLWNvbmZpZyhweXRob24zKSBtb3JlIHJvYnVzdAoKSXQgaXMgYSBwb3Nz
aWJpbGl0eSB0aGF0IHRoZSBpbnN0YWxsYXRpb24gY2FuJ3QgYmUgZm91bmQuCi0tLQogbWVzb24u
YnVpbGQgfCAxNCArKysrKysrKy0tLS0tLQogMSBmaWxlIGNoYW5nZWQsIDggaW5zZXJ0aW9ucygr
KSwgNiBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9tZXNvbi5idWlsZCBiL21lc29uLmJ1aWxk
CmluZGV4IDU2ZmJmNzUyZDIuLmRkZDk0MWNjZmUgMTAwNjQ0Ci0tLSBhL21lc29uLmJ1aWxkCisr
KyBiL21lc29uLmJ1aWxkCkBAIC0xMDUwLDE1ICsxMDUwLDE3IEBAIGVuZGlmCiAjIyMjIyMjIyMj
IyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMKIAog
cHlvcHQgPSBnZXRfb3B0aW9uKCdwbHB5dGhvbicpCitweXRob24zX2RlcCA9IG5vdF9mb3VuZF9k
ZXAKIGlmIG5vdCBweW9wdC5kaXNhYmxlZCgpCiAgIHBtID0gaW1wb3J0KCdweXRob24nKQotICBw
eXRob24zX2luc3QgPSBwbS5maW5kX2luc3RhbGxhdGlvbihyZXF1aXJlZDogcHlvcHQuZW5hYmxl
ZCgpKQotICBweXRob24zX2RlcCA9IHB5dGhvbjNfaW5zdC5kZXBlbmRlbmN5KGVtYmVkOiB0cnVl
LCByZXF1aXJlZDogcHlvcHQuZW5hYmxlZCgpKQotICBpZiBub3QgY2MuY2hlY2tfaGVhZGVyKCdQ
eXRob24uaCcsIGRlcGVuZGVuY2llczogcHl0aG9uM19kZXAsIHJlcXVpcmVkOiBweW9wdC5lbmFi
bGVkKCkpCi0gICAgcHl0aG9uM19kZXAgPSBub3RfZm91bmRfZGVwCisgIHB5dGhvbjNfaW5zdCA9
IHBtLmZpbmRfaW5zdGFsbGF0aW9uKHJlcXVpcmVkOiBweW9wdCkKKyAgaWYgcHl0aG9uM19pbnN0
LmZvdW5kKCkKKyAgICBweXRob24zX2RlcCA9IHB5dGhvbjNfaW5zdC5kZXBlbmRlbmN5KGVtYmVk
OiB0cnVlLCByZXF1aXJlZDogcHlvcHQpCisgICAgIyBSZW1vdmUgdGhpcyBjaGVjayBhZnRlciB3
ZSBkZXBlbmQgb24gTWVzb24gPj0gMS4xLjAKKyAgICBpZiBub3QgY2MuY2hlY2tfaGVhZGVyKCdQ
eXRob24uaCcsIGRlcGVuZGVuY2llczogcHl0aG9uM19kZXAsIHJlcXVpcmVkOiBweW9wdCkKKyAg
ICAgIHB5dGhvbjNfZGVwID0gbm90X2ZvdW5kX2RlcAorICAgIGVuZGlmCiAgIGVuZGlmCi1lbHNl
Ci0gIHB5dGhvbjNfZGVwID0gbm90X2ZvdW5kX2RlcAogZW5kaWYKIAogCi0tIApUcmlzdGFuIFBh
cnRpbgpOZW9uIChodHRwczovL25lb24udGVjaCkKCg==
--2eca9e845ddb461adaf774511162360fbe1dac129c6f3f6dc37a19059948--