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)