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
>From 8995f525d8036ad0aad9486a9b26864d87ebbe9f Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Thu, 29 Jun 2023 09:32:40 -0700 Subject: [PATCH v16a 1/2] meson: Pass more feature option through to required kwargs That was already done in a lot of places, but not all. Author: Tristan Partin <tris...@neon.tech> Discussion: https://postgr.es/m/CSPIJVUDZFKX.3KHMOAVGF94RV@c3po --- meson.build | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/meson.build b/meson.build index fbec9979475..6661c1fefa2 100644 --- a/meson.build +++ b/meson.build @@ -769,8 +769,8 @@ endif icuopt = get_option('icu') if not icuopt.disabled() - icu = dependency('icu-uc', required: icuopt.enabled()) - icu_i18n = dependency('icu-i18n', required: icuopt.enabled()) + icu = dependency('icu-uc', required: icuopt) + icu_i18n = dependency('icu-i18n', required: icuopt) if icu.found() cdata.set('USE_ICU', 1) @@ -1054,9 +1054,9 @@ endif pyopt = get_option('plpython') if not pyopt.disabled() pm = import('python') - python3_inst = pm.find_installation(required: pyopt.enabled()) - python3_dep = python3_inst.dependency(embed: true, required: pyopt.enabled()) - if not cc.check_header('Python.h', dependencies: python3_dep, required: pyopt.enabled()) + python3_inst = pm.find_installation(required: pyopt) + python3_dep = python3_inst.dependency(embed: true, required: pyopt) + if not cc.check_header('Python.h', dependencies: python3_dep, required: pyopt) python3_dep = not_found_dep endif else @@ -1079,7 +1079,7 @@ if not get_option('readline').disabled() readline = dependency(readline_dep, required: false) if not readline.found() readline = cc.find_library(readline_dep, - required: get_option('readline').enabled(), + required: get_option('readline'), dirs: test_lib_d) endif if readline.found() @@ -1379,7 +1379,7 @@ if not zlibopt.disabled() warning('did not find zlib') elif not cc.has_header('zlib.h', args: test_c_args, include_directories: postgres_inc, - dependencies: [zlib_t], required: zlibopt.enabled()) + dependencies: [zlib_t], required: zlibopt) warning('zlib header not found') elif not cc.has_type('z_streamp', dependencies: [zlib_t], prefix: '#include <zlib.h>', @@ -2538,7 +2538,7 @@ if not nlsopt.disabled() # otherwise there'd be lots of # "Gettext not found, all translation (po) targets will be ignored." # warnings if not found. - msgfmt = find_program('msgfmt', required: nlsopt.enabled(), native: true) + msgfmt = find_program('msgfmt', required: nlsopt, native: true) # meson 0.59 has this wrapped in dependency('intl') if (msgfmt.found() and -- 2.38.0
>From 2f681029390675ae1902bd209657bd4f063d646a Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Thu, 29 Jun 2023 09:42:09 -0700 Subject: [PATCH v16a 2/2] meson: Tie adding C++ support to the llvm Meson option In the event the llvm option is defined to be 'auto', it is possible that the host machine might not have a C++ compiler. If that is the case, then we shouldn't continue reaching for the llvm dependency. To make it easier to understand the case where LLVM support is disabled due to lacking a C++ compiler, add a message noting that fact. Author: Tristan Partin <tris...@neon.tech> Reviewed-by: Andres Freund <and...@anarazel.de> Discussion: https://postgr.es/m/CSPIJVUDZFKX.3KHMOAVGF94RV@c3po --- meson.build | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/meson.build b/meson.build index 6661c1fefa2..3ac5acb1732 100644 --- a/meson.build +++ b/meson.build @@ -742,8 +742,8 @@ endif ############################################################### llvmopt = get_option('llvm') -if not llvmopt.disabled() - add_languages('cpp', required: true, native: false) +llvm = not_found_dep +if add_languages('cpp', required: llvmopt, native: false) llvm = dependency('llvm', version: '>=3.9', method: 'config-tool', required: llvmopt) if llvm.found() @@ -757,8 +757,8 @@ if not llvmopt.disabled() ccache = find_program('ccache', native: true, required: false) clang = find_program(llvm_binpath / 'clang', required: true) endif -else - llvm = not_found_dep +elif llvmopt.auto() + message('llvm requires a C++ compiler') endif -- 2.38.0