[PATCH] Fix OAuth feature detection on OpenBSD+Meson

Started by Jacob Champion7 months ago18 messages
#1Jacob Champion
jacob.champion@enterprisedb.com
1 attachment(s)

Hi all,

I noticed that the OpenBSD build in CI wasn't running the libcurl
tests. Turns out the feature test I added in b0635bfda is subtly
broken, because it uses cc.check_header() rather than cc.has_header().
On OpenBSD, apparently, the <sys/event.h> header can't be compiled
without including additional prerequisite headers.

The attached patch fixes that by just switching to has_header(). (I
could add the prerequisites to the test instead, but I'd prefer to
exactly match the logic we use to determine the value of the
HAVE_SYS_EVENT_H macro. I don't think I had a good reason not to in
the first place.)

As a potential follow-up, is there any interest in switching the
Cirrus Meson setup to explicitly enable the features that we expect to
test? That would match what we do for Autoconf, I think; it would have
helped me catch my mistake earlier.

Thanks,
--Jacob

Attachments:

0001-oauth-Fix-kqueue-detection-on-OpenBSD.patchapplication/octet-stream; name=0001-oauth-Fix-kqueue-detection-on-OpenBSD.patchDownload
From f8dde8b1260a4b8d7a546af1ceed2a26c4bac35f Mon Sep 17 00:00:00 2001
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Fri, 13 Jun 2025 15:52:07 -0700
Subject: [PATCH] oauth: Fix kqueue detection on OpenBSD

In b0635bfda, I added an early header check to the Meson OAuth support,
which was intended to duplicate the later checks for
HAVE_SYS_[EVENT|EPOLL]_H. However, I implemented the new test via
check_header() -- which tries to compile -- rather than has_header(),
which just looks for the file's existence.

The distinction matters on OpenBSD, where <sys/event.h> can't be
compiled without including prerequisite headers, so -Dlibcurl=enabled
failed on that platform. Switch to has_header() to fix this.
---
 meson.build | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/meson.build b/meson.build
index 06382b728e6..16a9c4708ed 100644
--- a/meson.build
+++ b/meson.build
@@ -943,10 +943,10 @@ if not libcurlopt.disabled()
   # libcurl and one of either epoll or kqueue.
   oauth_flow_supported = (
     libcurl.found()
-    and (cc.check_header('sys/event.h', required: false,
-                         args: test_c_args, include_directories: postgres_inc)
-         or cc.check_header('sys/epoll.h', required: false,
-                            args: test_c_args, include_directories: postgres_inc))
+    and (cc.has_header('sys/event.h',
+                       args: test_c_args, include_directories: postgres_inc)
+         or cc.has_header('sys/epoll.h',
+                          args: test_c_args, include_directories: postgres_inc))
   )
 
   if oauth_flow_supported
-- 
2.34.1

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Jacob Champion (#1)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

Jacob Champion <jacob.champion@enterprisedb.com> writes:

As a potential follow-up, is there any interest in switching the
Cirrus Meson setup to explicitly enable the features that we expect to
test? That would match what we do for Autoconf, I think; it would have
helped me catch my mistake earlier.

As far as I recall, we've always thought that autoconf's approach
of "explicitly specify the features you expect to get" is the
right way to do things. I don't love meson's default you-get-
whatever-seems-available approach at all, though maybe that's
my history as a package builder talking. I think not using that
behavior would be a real good idea, at least for testing purposes.

regards, tom lane

#3Nazir Bilal Yavuz
byavuz81@gmail.com
In reply to: Jacob Champion (#1)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

Hi,

On Tue, 24 Jun 2025 at 02:37, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

As a potential follow-up, is there any interest in switching the
Cirrus Meson setup to explicitly enable the features that we expect to
test? That would match what we do for Autoconf, I think; it would have
helped me catch my mistake earlier.

I think this is a good idea. Another point is that CI images and their
packages are updated automatically, so it would be easier to catch if
something breaks when the VM is updated.

--
Regards,
Nazir Bilal Yavuz
Microsoft

#4Peter Eisentraut
peter@eisentraut.org
In reply to: Jacob Champion (#1)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

On 24.06.25 01:36, Jacob Champion wrote:

I noticed that the OpenBSD build in CI wasn't running the libcurl
tests. Turns out the feature test I added in b0635bfda is subtly
broken, because it uses cc.check_header() rather than cc.has_header().
On OpenBSD, apparently, the <sys/event.h> header can't be compiled
without including additional prerequisite headers.

The attached patch fixes that by just switching to has_header(). (I
could add the prerequisites to the test instead, but I'd prefer to
exactly match the logic we use to determine the value of the
HAVE_SYS_EVENT_H macro. I don't think I had a good reason not to in
the first place.)

Note that Autoconf uses a compilation test, not a preprocessor test, for
its AC_CHECK_HEADERS, so it uses .check_header() semantics. And this
was the result of a long transition, because the compile test was
ultimately deemed to be better. So in general, I would be wary about
moving away from .check_header() toward .has_header(). But it looks
like meson.build mixes those without much of a pattern, so maybe it
doesn't matter for now.

But I'm also suspicious, because by this explanation, the
AC_CHECK_HEADERS calls on sys/event.h should fail on OpenBSD, but they
do not on the existing buildfarm members.

#5Jacob Champion
jacob.champion@enterprisedb.com
In reply to: Peter Eisentraut (#4)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

On Tue, Jun 24, 2025 at 1:29 PM Peter Eisentraut <peter@eisentraut.org> wrote:

Note that Autoconf uses a compilation test, not a preprocessor test, for
its AC_CHECK_HEADERS, so it uses .check_header() semantics. And this
was the result of a long transition, because the compile test was
ultimately deemed to be better. So in general, I would be wary about
moving away from .check_header() toward .has_header(). But it looks
like meson.build mixes those without much of a pattern, so maybe it
doesn't matter for now.

I don't mind moving in that direction, but I do want the two sides to
match. So if it was good enough up to this point to use has_header()
for our feature macros, I don't think I want to try to change that for
18.

But I'm also suspicious, because by this explanation, the
AC_CHECK_HEADERS calls on sys/event.h should fail on OpenBSD, but they
do not on the existing buildfarm members.

I think Andres tracked that discrepancy down [1]/messages/by-id/637haqqyhg2wlz7q6wq25m2qupe67g7f2uupngzui64zypy4x2@ysr2xnmynmu4:

Gah, configure does pass - because AC_CHECK_HEADER(), if includes is not passed
in, first includes what's defined in AC_INCLUDES_DEFAULT.

Thanks!
--Jacob

[1]: /messages/by-id/637haqqyhg2wlz7q6wq25m2qupe67g7f2uupngzui64zypy4x2@ysr2xnmynmu4

#6Jacob Champion
jacob.champion@enterprisedb.com
In reply to: Tom Lane (#2)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

On Mon, Jun 23, 2025 at 5:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

As far as I recall, we've always thought that autoconf's approach
of "explicitly specify the features you expect to get" is the
right way to do things. I don't love meson's default you-get-
whatever-seems-available approach at all, though maybe that's
my history as a package builder talking. I think not using that
behavior would be a real good idea, at least for testing purposes.

For development I like "give me everything" by default, but for
testing and packaging I agree with you.

--Jacob

#7Jacob Champion
jacob.champion@enterprisedb.com
In reply to: Nazir Bilal Yavuz (#3)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

On Tue, Jun 24, 2025 at 1:27 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:

I think this is a good idea. Another point is that CI images and their
packages are updated automatically, so it would be easier to catch if
something breaks when the VM is updated.

Yes, that's a great point too. Okay, sounds like there is some
interest, and I'll add it to my list of patchsets to try (but if
anyone wants to beat me to it, please go ahead!).

--Jacob

#8Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#4)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

Peter Eisentraut <peter@eisentraut.org> writes:

Note that Autoconf uses a compilation test, not a preprocessor test, for
its AC_CHECK_HEADERS, so it uses .check_header() semantics. And this
was the result of a long transition, because the compile test was
ultimately deemed to be better. So in general, I would be wary about
moving away from .check_header() toward .has_header(). But it looks
like meson.build mixes those without much of a pattern, so maybe it
doesn't matter for now.

But I'm also suspicious, because by this explanation, the
AC_CHECK_HEADERS calls on sys/event.h should fail on OpenBSD, but they
do not on the existing buildfarm members.

I was curious about this, so I tried it on a handy OpenBSD 7.7
installation. Indeed, sys/event.h does not compile on its own:

$ cat tst.c
#include <sys/event.h>

int main() { return 0; }

$ cc tst.c
In file included from tst.c:1:
/usr/include/sys/event.h:57:2: error: unknown type name '__uintptr_t'; did you mean '__uint128_t'?
__uintptr_t ident; /* identifier for this event */
^
note: '__uint128_t' declared here
/usr/include/sys/event.h:61:2: error: unknown type name '__int64_t'; did you mean '__int128_t'?
__int64_t data; /* filter data value */
^
note: '__int128_t' declared here
2 errors generated.

Whether this is intentional is hard to say, because I can't find
either event.h or any of the functions it declares in the OpenBSD
man pages. But anyway, AC_CHECK_HEADERS does think that <sys/event.h>
is available, and that's because it does NOT just blindly include
the header-to-test. It includes assorted standard headers
such as <stdint.h> first, thus dodging the problem.

I confirm Jacob's result that our meson.build fails to think
that <sys/event.h> is available, so we do need to do something.
I'm not excited about dropping the compilability check though.
If meson can't do that more like autoconf does it, I foresee
needing to build ad-hoc reimplementations of autoconf's logic.

regards, tom lane

#9Jacob Champion
jacob.champion@enterprisedb.com
In reply to: Tom Lane (#8)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

On Tue, Jun 24, 2025 at 2:03 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I confirm Jacob's result that our meson.build fails to think
that <sys/event.h> is available, so we do need to do something.

(To clarify for other readers: it's the OAuth feature test I added
that fails. The existing test for HAVE_SYS_EVENT_H is working fine. I
accidentally made mine stricter.)

--Jacob

#10Tom Lane
tgl@sss.pgh.pa.us
In reply to: Jacob Champion (#9)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

Jacob Champion <jacob.champion@enterprisedb.com> writes:

On Tue, Jun 24, 2025 at 2:03 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I confirm Jacob's result that our meson.build fails to think
that <sys/event.h> is available, so we do need to do something.

(To clarify for other readers: it's the OAuth feature test I added
that fails. The existing test for HAVE_SYS_EVENT_H is working fine. I
accidentally made mine stricter.)

Ah, you're correct: I saw "Check usable header "sys/event.h" : NO"
in the meson log, but that came from the check in the
oauth_flow_supported stanza. We do end up with

build/src/include/pg_config.h:#define HAVE_SYS_EVENT_H 1

after a second test that tries to compile

#ifdef __has_include
#if !__has_include("sys/event.h")
#error "Header 'sys/event.h' could not be found"
#endif
#else
#include <sys/event.h>
#endif

Can't say that I find this to be impressive software engineering:
rather than having only one probe failure mode to worry about,
we have two, depending on whether the compiler knows __has_include().
Pretty close to the worst of all possible worlds.

regards, tom lane

#11Jacob Champion
jacob.champion@enterprisedb.com
In reply to: Tom Lane (#10)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

On Tue, Jun 24, 2025 at 2:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Can't say that I find this to be impressive software engineering:
rather than having only one probe failure mode to worry about,
we have two, depending on whether the compiler knows __has_include().
Pretty close to the worst of all possible worlds.

I did a double-take on the code you posted, but! It looks like they're
running the preprocessor only. That doesn't seem so bad to me (though
they could probably do better than calling it a "compile" in the log).

--Jacob

#12Tom Lane
tgl@sss.pgh.pa.us
In reply to: Jacob Champion (#11)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

Jacob Champion <jacob.champion@enterprisedb.com> writes:

On Tue, Jun 24, 2025 at 2:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Can't say that I find this to be impressive software engineering:
rather than having only one probe failure mode to worry about,
we have two, depending on whether the compiler knows __has_include().
Pretty close to the worst of all possible worlds.

I did a double-take on the code you posted, but! It looks like they're
running the preprocessor only. That doesn't seem so bad to me (though
they could probably do better than calling it a "compile" in the log).

Oh! Okay, then the two cases should be mostly semantically
equivalent, I think, ie it's just a "does the header exist" test.
There are probably some edge cases where the effects are different,
but nothing that would be likely to matter for us.

regards, tom lane

#13Peter Eisentraut
peter@eisentraut.org
In reply to: Jacob Champion (#5)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

On 24.06.25 22:39, Jacob Champion wrote:

On Tue, Jun 24, 2025 at 1:29 PM Peter Eisentraut <peter@eisentraut.org> wrote:

Note that Autoconf uses a compilation test, not a preprocessor test, for
its AC_CHECK_HEADERS, so it uses .check_header() semantics. And this
was the result of a long transition, because the compile test was
ultimately deemed to be better. So in general, I would be wary about
moving away from .check_header() toward .has_header(). But it looks
like meson.build mixes those without much of a pattern, so maybe it
doesn't matter for now.

I don't mind moving in that direction, but I do want the two sides to
match. So if it was good enough up to this point to use has_header()
for our feature macros, I don't think I want to try to change that for
18.

right

But I'm also suspicious, because by this explanation, the
AC_CHECK_HEADERS calls on sys/event.h should fail on OpenBSD, but they
do not on the existing buildfarm members.

I think Andres tracked that discrepancy down [1]:

Gah, configure does pass - because AC_CHECK_HEADER(), if includes is not passed
in, first includes what's defined in AC_INCLUDES_DEFAULT.

Ah, that explains it.

#14Nazir Bilal Yavuz
byavuz81@gmail.com
In reply to: Jacob Champion (#7)
2 attachment(s)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

Hi,

On Tue, 24 Jun 2025 at 23:46, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

On Tue, Jun 24, 2025 at 1:27 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:

I think this is a good idea. Another point is that CI images and their
packages are updated automatically, so it would be easier to catch if
something breaks when the VM is updated.

Yes, that's a great point too. Okay, sounds like there is some
interest, and I'll add it to my list of patchsets to try (but if
anyone wants to beat me to it, please go ahead!).

I wanted to experiment with it. First, I got the current list of
features from upstream, then disabled the auto features, then
explicitly enabled these features. I did this only for FreeBSD to show
my idea roughly.

There are two patches; 0001 disables auto features for all of the
tasks and 0002 explicitly enables these features for FreeBSD.

What do you think about this approach? If you are okay with this, I
can apply it to all CI tasks (and possibly create another thread for
it).

--
Regards,
Nazir Bilal Yavuz
Microsoft

Attachments:

v1-0001-ci-Disable-meson-auto-features.patchtext/x-patch; charset=US-ASCII; name=v1-0001-ci-Disable-meson-auto-features.patchDownload
From e57cf4f5a23312d81b13b4f7cbc7bb728b701621 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <byavuz81@gmail.com>
Date: Wed, 25 Jun 2025 10:41:26 +0300
Subject: [PATCH v1 1/2] ci: Disable meson auto features

---
 .cirrus.tasks.yml | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index 92057006c93..4daaa8ea17b 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -30,6 +30,7 @@ env:
   PGCTLTIMEOUT: 120 # avoids spurious failures during parallel tests
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl libpq_encryption load_balance oauth
+  MESON_AUTO_FEATURES: disabled
 
 
 # What files to preserve in case tests fail
@@ -198,6 +199,7 @@ task:
       meson setup \
         --buildtype=debug \
         -Dcassert=true -Dinjection_points=true \
+        --auto-features=${MESON_AUTO_FEATURES} \
         -Duuid=bsd -Dtcl_version=tcl86 -Ddtrace=auto \
         -Dextra_lib_dirs=/usr/local/lib -Dextra_include_dirs=/usr/local/include/ \
         build
@@ -317,6 +319,7 @@ task:
         --buildtype=debugoptimized \
         --pkg-config-path ${PKGCONFIG_PATH} \
         -Dcassert=true -Dinjection_points=true \
+        --auto-features=${MESON_AUTO_FEATURES} \
         -Dssl=openssl ${UUID} ${TCL} \
         -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
         build
@@ -508,6 +511,7 @@ task:
           meson setup \
             --buildtype=debug \
             -Dcassert=true -Dinjection_points=true \
+            --auto-features=${MESON_AUTO_FEATURES} \
             ${LINUX_MESON_FEATURES} \
             build
         EOF
@@ -520,6 +524,7 @@ task:
           meson setup \
             --buildtype=debug \
             -Dcassert=true -Dinjection_points=true \
+            --auto-features=${MESON_AUTO_FEATURES} \
             ${LINUX_MESON_FEATURES} \
             -Dllvm=disabled \
             --pkg-config-path /usr/lib/i386-linux-gnu/pkgconfig/ \
@@ -661,6 +666,7 @@ task:
       -Dextra_include_dirs=/opt/local/include \
       -Dextra_lib_dirs=/opt/local/lib \
       -Dcassert=true -Dinjection_points=true \
+      --auto-features=${MESON_AUTO_FEATURES} \
       -Duuid=e2fs -Ddtrace=auto \
       build
 
@@ -733,7 +739,7 @@ task:
   # Use /DEBUG:FASTLINK to avoid high memory usage during linking
   configure_script: |
     vcvarsall x64
-    meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Dinjection_points=true -Db_pch=true -Dextra_lib_dirs=c:\openssl\1.1\lib -Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR% build
+    meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Dinjection_points=true -Db_pch=true -Dextra_lib_dirs=c:\openssl\1.1\lib -Dextra_include_dirs=c:\openssl\1.1\include --auto-features=%MESON_AUTO_FEATURES% -DTAR=%TAR% build
 
   build_script: |
     vcvarsall x64
@@ -793,7 +799,7 @@ task:
 
   # disable -Dnls as the number of files it creates cause a noticable slowdown
   configure_script: |
-    %BASH% -c "meson setup -Ddebug=true -Doptimization=g -Dcassert=true -Dinjection_points=true -Db_pch=true -Dnls=disabled -DTAR=%TAR% build"
+    %BASH% -c "meson setup -Ddebug=true -Doptimization=g -Dcassert=true -Dinjection_points=true -Db_pch=true -Dnls=disabled --auto-features=%MESON_AUTO_FEATURES% -DTAR=%TAR% build"
 
   build_script: |
     %BASH% -c "ninja -C build ${MBUILD_TARGET}"
-- 
2.49.0

v1-0002-ci-freebsd-Explicitly-enable-meson-features.patchtext/x-patch; charset=US-ASCII; name=v1-0002-ci-freebsd-Explicitly-enable-meson-features.patchDownload
From 248aeacbebc29ddd0a3cb3a816209a0780b854e9 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <byavuz81@gmail.com>
Date: Wed, 25 Jun 2025 11:14:26 +0300
Subject: [PATCH v1 2/2] ci: freebsd: Explicitly enable meson features

ci-os-only: freebsd
---
 .cirrus.tasks.yml | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index 4daaa8ea17b..9dab6a1ebdf 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -165,6 +165,29 @@ task:
       -c debug_parallel_query=regress
     PG_TEST_PG_UPGRADE_MODE: --link
 
+    MESON_FEATURES: >-
+      -Ddtrace=enabled
+      -Dtap_tests=enabled
+      -Ddocs=enabled
+      -Dgssapi=enabled
+      -Dicu=enabled
+      -Dldap=enabled
+      -Dlibcurl=enabled
+      -Dlibxml=enabled
+      -Dlibxslt=enabled
+      -Dlz4=enabled
+      -Dnls=enabled
+      -Dssl=openssl
+      -Dpam=enabled
+      -Dplperl=enabled
+      -Dplpython=enabled
+      -Dtcl_version=tcl86
+      -Dpltcl=enabled
+      -Dreadline=enabled
+      -Duuid=bsd
+      -Dzlib=enabled
+      -Dzstd=enabled
+
   <<: *freebsd_task_template
 
   depends_on: SanityCheck
@@ -199,8 +222,7 @@ task:
       meson setup \
         --buildtype=debug \
         -Dcassert=true -Dinjection_points=true \
-        --auto-features=${MESON_AUTO_FEATURES} \
-        -Duuid=bsd -Dtcl_version=tcl86 -Ddtrace=auto \
+        --auto-features=${MESON_AUTO_FEATURES} ${MESON_FEATURES} \
         -Dextra_lib_dirs=/usr/local/lib -Dextra_include_dirs=/usr/local/include/ \
         build
     EOF
-- 
2.49.0

#15Jacob Champion
jacob.champion@enterprisedb.com
In reply to: Nazir Bilal Yavuz (#14)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

On Wed, Jun 25, 2025 at 3:46 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:

I wanted to experiment with it.

That was fast, thank you!

First, I got the current list of
features from upstream, then disabled the auto features, then
explicitly enabled these features. I did this only for FreeBSD to show
my idea roughly.

There are two patches; 0001 disables auto features for all of the
tasks and 0002 explicitly enables these features for FreeBSD.

Just from a stylistic perspective, I think I'd prefer for
`--auto-features=disabled` to be folded into the beginning of the
MESON_FEATURES variable. Or is there another reason to control them
fully independently?

Also: how do we ensure that none of the previously enabled features
were missed in this list, for all the OSes we've modified? (I'm a
Meson novice.)

What do you think about this approach? If you are okay with this, I
can apply it to all CI tasks (and possibly create another thread for
it).

Seems good to me. I think a new top-level thread would be good too, to
get more eyes on the changes.

Thanks!
--Jacob

#16Nazir Bilal Yavuz
byavuz81@gmail.com
In reply to: Jacob Champion (#15)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

Hi,

On Mon, 30 Jun 2025 at 17:45, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

On Wed, Jun 25, 2025 at 3:46 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:

I wanted to experiment with it.

That was fast, thank you!

First, I got the current list of
features from upstream, then disabled the auto features, then
explicitly enabled these features. I did this only for FreeBSD to show
my idea roughly.

There are two patches; 0001 disables auto features for all of the
tasks and 0002 explicitly enables these features for FreeBSD.

Just from a stylistic perspective, I think I'd prefer for
`--auto-features=disabled` to be folded into the beginning of the
MESON_FEATURES variable. Or is there another reason to control them
fully independently?

No, I had thought people would prefer to control auto-features from
one central place but apparently I was wrong :) I will update the
patch according to this.

Also: how do we ensure that none of the previously enabled features
were missed in this list, for all the OSes we've modified? (I'm a
Meson novice.)

I am not sure, I do not know if there is a way to ensure that. I just
manually checked features from the end of the 'meson setup' command's
output. I think this should be enough but of course an automated way
would be better if there is any.

What do you think about this approach? If you are okay with this, I
can apply it to all CI tasks (and possibly create another thread for
it).

Seems good to me. I think a new top-level thread would be good too, to
get more eyes on the changes.

Thanks! I will create the thread soon.

--
Regards,
Nazir Bilal Yavuz
Microsoft

#17Nazir Bilal Yavuz
byavuz81@gmail.com
In reply to: Nazir Bilal Yavuz (#14)
1 attachment(s)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

Hi,

On Wed, 25 Jun 2025 at 13:45, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:

Hi,

On Tue, 24 Jun 2025 at 23:46, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

On Tue, Jun 24, 2025 at 1:27 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:

I think this is a good idea. Another point is that CI images and their
packages are updated automatically, so it would be easier to catch if
something breaks when the VM is updated.

Yes, that's a great point too. Okay, sounds like there is some
interest, and I'll add it to my list of patchsets to try (but if
anyone wants to beat me to it, please go ahead!).

I wanted to experiment with it. First, I got the current list of
features from upstream, then disabled the auto features, then
explicitly enabled these features. I did this only for FreeBSD to show
my idea roughly.

There are two patches; 0001 disables auto features for all of the
tasks and 0002 explicitly enables these features for FreeBSD.

It seems CFBot is using these patches [1]https://github.com/postgresql-cfbot/postgresql/commit/453329f784a7977099046f94ec45e9614c9ab279, resharing actual patch [2]/messages/by-id/CAOYmi+kdR218ke2zu74oTJvzYJcqV1MN5=mGAPqZQuc79HMSVA@mail.gmail.com
with the hopes that it will be used by CFBot.

[1]: https://github.com/postgresql-cfbot/postgresql/commit/453329f784a7977099046f94ec45e9614c9ab279
[2]: /messages/by-id/CAOYmi+kdR218ke2zu74oTJvzYJcqV1MN5=mGAPqZQuc79HMSVA@mail.gmail.com

--
Regards,
Nazir Bilal Yavuz
Microsoft

Attachments:

0001-oauth-Fix-kqueue-detection-on-OpenBSD.patchtext/x-patch; charset=US-ASCII; name=0001-oauth-Fix-kqueue-detection-on-OpenBSD.patchDownload
From f8dde8b1260a4b8d7a546af1ceed2a26c4bac35f Mon Sep 17 00:00:00 2001
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Fri, 13 Jun 2025 15:52:07 -0700
Subject: [PATCH] oauth: Fix kqueue detection on OpenBSD

In b0635bfda, I added an early header check to the Meson OAuth support,
which was intended to duplicate the later checks for
HAVE_SYS_[EVENT|EPOLL]_H. However, I implemented the new test via
check_header() -- which tries to compile -- rather than has_header(),
which just looks for the file's existence.

The distinction matters on OpenBSD, where <sys/event.h> can't be
compiled without including prerequisite headers, so -Dlibcurl=enabled
failed on that platform. Switch to has_header() to fix this.
---
 meson.build | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/meson.build b/meson.build
index 06382b728e6..16a9c4708ed 100644
--- a/meson.build
+++ b/meson.build
@@ -943,10 +943,10 @@ if not libcurlopt.disabled()
   # libcurl and one of either epoll or kqueue.
   oauth_flow_supported = (
     libcurl.found()
-    and (cc.check_header('sys/event.h', required: false,
-                         args: test_c_args, include_directories: postgres_inc)
-         or cc.check_header('sys/epoll.h', required: false,
-                            args: test_c_args, include_directories: postgres_inc))
+    and (cc.has_header('sys/event.h',
+                       args: test_c_args, include_directories: postgres_inc)
+         or cc.has_header('sys/epoll.h',
+                          args: test_c_args, include_directories: postgres_inc))
   )
 
   if oauth_flow_supported
-- 
2.34.1

#18Jacob Champion
jacob.champion@enterprisedb.com
In reply to: Nazir Bilal Yavuz (#17)
Re: [PATCH] Fix OAuth feature detection on OpenBSD+Meson

On Mon, Jul 7, 2025 at 5:41 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:

It seems CFBot is using these patches [1], resharing actual patch [2]
with the hopes that it will be used by CFBot.

Whoops, thanks for reposting that. I plan to get this pushed today so
that it doesn't start blocking the Meson work.

--Jacob