Feedback-ID: id4a34324:Fastmail
Date: Thu, 29 Jun 2023 10:35:32 -0700
From: Andres Freund <andres@anarazel.de>
To: Peter Eisentraut <peter@eisentraut.org>
Cc: Tristan Partin <tristan@neon.tech>, pgsql-hackers@postgresql.org
Subject: Re: Meson build updates
Message-ID: <20230629173532.cdcydvqf3x5j7524@awork3.anarazel.de>
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>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="owu2kol6rl73t573"
Content-Disposition: inline
In-Reply-To: <fc86a216-4827-be14-2468-7309ea189f67@eisentraut.org>
Archived-At: 
 <https://www.postgresql.org/message-id/20230629173532.cdcydvqf3x5j7524%40awork3.anarazel.de>
Precedence: bulk


--owu2kol6rl73t573
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--owu2kol6rl73t573
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v16a-0001-meson-Pass-more-feature-option-through-to-requi.patch"

From 8995f525d8036ad0aad9486a9b26864d87ebbe9f Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@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 <tristan@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


--owu2kol6rl73t573
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v16a-0002-meson-Tie-adding-C-support-to-the-llvm-Meson-op.patch"

From 2f681029390675ae1902bd209657bd4f063d646a Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@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 <tristan@neon.tech>
Reviewed-by: Andres Freund <andres@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


--owu2kol6rl73t573--