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