Thread: 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.
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]: > 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] https://www.postgresql.org/message-id/637haqqyhg2wlz7q6wq25m2qupe67g7f2uupngzui64zypy4x2%40ysr2xnmynmu4
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
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
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
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
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
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.