meson: pass OpenSSL/ICU include dirs to extensions
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253717psql -h localhost -U postgresBuilt from patchset v9 (message #9), October 26, 2026 at 06:58 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253717_9 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253717_9 && git checkout t253717_9Patchset v9 (message #9) is on t253717_9
Hi,
I was building an extension against a meson-built tree on macOS and the
build failed because a public server header includes openssl/ssl.h, but
the compiler couldn't find it. The extension includes libpq/auth.h,
which pulls in libpq-be.h, which includes openssl/ssl.h when OpenSSL
support is enabled. On systems where OpenSSL is installed in a
non-default prefix (Homebrew on macOS being the common case), that header
is not on the default search path.
With autoconf this works because you pass the include path to configure
via CPPFLAGS and it ends up in Makefile.global and pg_config --cppflags.
But meson keeps the include directory inside the ssl dependency object
and never writes it into var_cppflags, so extensions get nothing.
ICU has the same issue, and it's actually been known since the original
meson commit: ICU_CFLAGS sits in pgxs_empty with a comment saying it
"needs to be added, included by public server headers", so the
$(ICU_CFLAGS) reference in Makefile.global has always been empty.
The attached patch fixes both, mirroring how autoconf handles each.
For OpenSSL I extract the include directory from the ssl dependency and
append it to var_cppflags in src/include/meson.build. Since var_cppflags
feeds both VAL_CPPFLAGS and CPPFLAGS in the generated Makefile.global,
this fixes pg_config --cppflags and the PGXS build in one place, which
matches autoconf (where OpenSSL rides along in CPPFLAGS).
For ICU I populate ICU_CFLAGS from the icu dependency and drop it from
pgxs_empty. ICU gets a cmake fallback in get_variable because the meson
setup can find ICU via cmake; OpenSSL can't, so it doesn't get one.
One thing I noticed and left alone: autoconf's pg_config --cppflags also
includes ICU_CFLAGS (VAL_CPPFLAGS is built from STD_CPPFLAGS, which picks
up the override), while meson's won't. Extensions don't care because they
get ICU through $(ICU_CFLAGS) in Makefile.global, and I didn't want to
duplicate the flags in the PGXS path. Easy to add ICU to var_cppflags
too if you want full parity there.
I tested by building and installing PostgreSQL both ways (autoconf and
meson, with SSL enabled, on macOS with Homebrew) and compiling the same
extension against each installation. Both build now, and the compile
command contains both the OpenSSL and ICU include directories. Before
the patch, meson's pg_config --cppflags was empty for these.
Regards,
Aidar Imamov
Hi,
On Tue, Sep 08, 2026 at 07:37:01PM +0000, Aidar Imamov wrote:
Some external dependencies have headers that are included by public
server headers (e.g. libpq-be.h includes openssl/ssl.h).
Thanks for picking this up! The problem is real, and the ICU half of
it has been sitting there since the Meson PGXS layer went in -- the
ICU_CFLAGS entry in pgxs_empty even carries a comment saying it still
needs to be done.
I do have some questions about how the include flags are extracted.
None of them are about the direction, which I think is right.
The main one is about
dependency.get_variable(pkgconfig: 'includedir'). That gives us the
raw pkg-config variable rather than the compiler arguments Meson
itself used for the dependency, and the two need not be equivalent: there may
be several include directories, or flags that aren't a plain
-I<includedir>, or, in a cross build, paths where
PKG_CONFIG_SYSROOT_DIR may not be applied to the raw includedir value
in the same way as to the compiler flags.
I looked for a way to take the flags directly instead. LLVM does
something along those lines, reading cxxflags from llvm-config rather
than rebuilding -I from a directory, but pkg-config does not seem to
expose Cflags through get_variable() in the same way, so this probably
needs a different approach. I don't have a good suggestion yet.
The ICU CMake fallback shows the difficulty concretely:
icu_incdir.split(';')[0]
ICU_INCLUDE_DIRS can hold more than one directory, so I think we would
want to keep all of them rather than just the first.
The OpenSSL part also appears not to cover the CMake discovery method.
In that case
ssl.get_variable(pkgconfig: 'includedir', default_value: '')
falls back to the empty default, even though the dependency may well
have non-default include directories. The cc.find_library() path is a
different situation, and extra_include_dirs never reaching
var_cppflags is really a broader PGXS gap rather than something for
this patch. It would help me to know which discovery methods you are
aiming to cover.
A much smaller thing: '-I' concatenated with a raw directory may need
escaping before it lands in Makefile.global, in case the prefix
contains whitespace or characters that mean something to make or the
shell.
For what it's worth, I agree with putting OpenSSL in CPPFLAGS and ICU
in ICU_CFLAGS. That matches how Makefile.global is structured, and it
might be worth spelling out in the commit message, since it is not
obvious why the two are handled differently.
Would an automated check of the generated PGXS flags be feasible, with
the dependency in a non-default prefix?
Mostly this comes back to one thing: having the flags follow what
Meson actually used for the dependency, instead of rebuilding them
from a directory variable. If a next version does that and keeps all
the include directories, I think the approach is sound. Happy to look
again whenever you have something!
Thanks,
Yuriy Grigoryev
________________________________________
От: Aidar Imamov <imamovaj22@gmail.com>
Отправлено: 9 сентября 2026 г. 2:37:01
Кому: pgsql-hackers@postgresql.org
Тема: [MASSMAIL]meson: pass OpenSSL/ICU include dirs to extensions
Hi,
I was building an extension against a meson-built tree on macOS and the
build failed because a public server header includes openssl/ssl.h, but
the compiler couldn't find it. The extension includes libpq/auth.h,
which pulls in libpq-be.h, which includes openssl/ssl.h when OpenSSL
support is enabled. On systems where OpenSSL is installed in a
non-default prefix (Homebrew on macOS being the common case), that header
is not on the default search path.
With autoconf this works because you pass the include path to configure
via CPPFLAGS and it ends up in Makefile.global and pg_config --cppflags.
But meson keeps the include directory inside the ssl dependency object
and never writes it into var_cppflags, so extensions get nothing.
ICU has the same issue, and it's actually been known since the original
meson commit: ICU_CFLAGS sits in pgxs_empty with a comment saying it
"needs to be added, included by public server headers", so the
$(ICU_CFLAGS) reference in Makefile.global has always been empty.
The attached patch fixes both, mirroring how autoconf handles each.
For OpenSSL I extract the include directory from the ssl dependency and
append it to var_cppflags in src/include/meson.build. Since var_cppflags
feeds both VAL_CPPFLAGS and CPPFLAGS in the generated Makefile.global,
this fixes pg_config --cppflags and the PGXS build in one place, which
matches autoconf (where OpenSSL rides along in CPPFLAGS).
For ICU I populate ICU_CFLAGS from the icu dependency and drop it from
pgxs_empty. ICU gets a cmake fallback in get_variable because the meson
setup can find ICU via cmake; OpenSSL can't, so it doesn't get one.
One thing I noticed and left alone: autoconf's pg_config --cppflags also
includes ICU_CFLAGS (VAL_CPPFLAGS is built from STD_CPPFLAGS, which picks
up the override), while meson's won't. Extensions don't care because they
get ICU through $(ICU_CFLAGS) in Makefile.global, and I didn't want to
duplicate the flags in the PGXS path. Easy to add ICU to var_cppflags
too if you want full parity there.
I tested by building and installing PostgreSQL both ways (autoconf and
meson, with SSL enabled, on macOS with Homebrew) and compiling the same
extension against each installation. Both build now, and the compile
command contains both the OpenSSL and ICU include directories. Before
the patch, meson's pg_config --cppflags was empty for these.
Regards,
Aidar Imamov
On Sep 9, 2026, at 15:01, Grigorev Jurij <ju.grigorev@ftdata.ru> wrote:
Hi,
On Tue, Sep 08, 2026 at 07:37:01PM +0000, Aidar Imamov wrote:
Some external dependencies have headers that are included by public
server headers (e.g. libpq-be.h includes openssl/ssl.h).Thanks for picking this up! The problem is real, and the ICU half of
it has been sitting there since the Meson PGXS layer went in -- the
ICU_CFLAGS entry in pgxs_empty even carries a comment saying it still
needs to be done.I do have some questions about how the include flags are extracted.
None of them are about the direction, which I think is right.The main one is about
dependency.get_variable(pkgconfig: 'includedir'). That gives us the
raw pkg-config variable rather than the compiler arguments Meson
itself used for the dependency, and the two need not be equivalent: there may
be several include directories, or flags that aren't a plain
-I<includedir>, or, in a cross build, paths where
PKG_CONFIG_SYSROOT_DIR may not be applied to the raw includedir value
in the same way as to the compiler flags.I looked for a way to take the flags directly instead. LLVM does
something along those lines, reading cxxflags from llvm-config rather
than rebuilding -I from a directory, but pkg-config does not seem to
expose Cflags through get_variable() in the same way, so this probably
needs a different approach. I don't have a good suggestion yet.The ICU CMake fallback shows the difficulty concretely:
icu_incdir.split(';')[0]
ICU_INCLUDE_DIRS can hold more than one directory, so I think we would
want to keep all of them rather than just the first.The OpenSSL part also appears not to cover the CMake discovery method.
In that casessl.get_variable(pkgconfig: 'includedir', default_value: '')
falls back to the empty default, even though the dependency may well
have non-default include directories. The cc.find_library() path is a
different situation, and extra_include_dirs never reaching
var_cppflags is really a broader PGXS gap rather than something for
this patch. It would help me to know which discovery methods you are
aiming to cover.A much smaller thing: '-I' concatenated with a raw directory may need
escaping before it lands in Makefile.global, in case the prefix
contains whitespace or characters that mean something to make or the
shell.For what it's worth, I agree with putting OpenSSL in CPPFLAGS and ICU
in ICU_CFLAGS. That matches how Makefile.global is structured, and it
might be worth spelling out in the commit message, since it is not
obvious why the two are handled differently.Would an automated check of the generated PGXS flags be feasible, with
the dependency in a non-default prefix?Mostly this comes back to one thing: having the flags follow what
Meson actually used for the dependency, instead of rebuilding them
from a directory variable. If a next version does that and keeps all
the include directories, I think the approach is sound. Happy to look
again whenever you have something!Thanks,
Yuriy Grigoryev
Thanks for the review.
Right, rebuilding the -I flags from includedir was the weak part. The new
version asks the dependency's own tooling instead, same as we do for
python_includespec (and like the llvm-config thing). For pkg-config deps it
runs "pkg-config --cflags <module>" and takes the whole output, so it's what
pkg-config gives the compiler, not something I built myself. For cmake deps
it reads the include dirs from the cmake variable and keeps all of them, not
just the first one.
Both discovery paths are covered now: pkgconfig via pkg-config --cflags, cmake
via get_variable(cmake: ...). find_library I left alone, that's the bigger
PGXS hole and you said it's not this patch's problem anyway.
Fair point about OpenSSL coming from cmake. dependency('openssl') walks the
whole auto chain, not only pkg-config, so the old code did nothing in that
case. Added a cmake branch for it.
OpenSSL goes to CPPFLAGS and ICU to ICU_CFLAGS: it's just where
Makefile.global already expects them, same as autoconf.
Automated check: doable, but I'd do it as a follow-up, since it needs a dep
in a non-default prefix and the meson suite doesn't stand up third-party libs
yet.
One thing I didn't touch: cross builds. Running pkg-config ourselves skips the
sysroot handling that meson applies to a dep's flags. Same as the existing
python_includespec, so I kept it consistent instead of adding something new.
v2 attached. Thanks.
Regards,
Aidar Imamov
Hi Aidar,
Thanks for the quick update. v2 looks much closer to me!
I like that both pkgconfig and cmake are covered now, and that all include dirs are kept.
Placement still looks right too: OpenSSL in CPPFLAGS and ICU in ICU_CFLAGS,
matching Makefile.global and autoconf.
Three small worries, all about cases where flags end up silently empty:
First, the hand-rolled pkg-config call:
if ssl.type_name() == 'pkgconfig' and pkg_config.found()
openssl_cflags = run_command(pkg_config, '--cflags', 'openssl',
check: false).stdout().strip()
Could we check returncode() here, use native: true, and query "icu-uc icu-i18n" together like configure does?
Right now a failed pkg-config just gives empty flags, and the extension fails much later with a missing header.
Second, pkg_config is defined in one file but used in another:
src/include/meson.build:
pkg_config = find_program( 'pkg-config', required: false)
src/makefiles/meson.build:
if icu.type_name() == 'pkgconfig' and pkg_config.found()
It works today because of subdir ordering, but it is fragile.
Maybe move it up or look it up locally?
Third, the cmake branch:
foreach d : ssl.get_variable(cmake: 'OPENSSL_INCLUDE_DIR',
default_value: '').split(';')
Are OPENSSL_INCLUDE_DIR and ICU_INCLUDE_DIRS valid for
both Find-modules and upstream config files? With default_value: ''
a wrong name just means empty flags. Same for find_library deps --
I agree extra_include_dirs is a separate gap, but a short comment would help.
One question, not a blocker: with this split, pg_config --cppflags still misses ICU,
unlike autoconf. PGXS make is fine through $(ICU_CFLAGS), but direct
pg_config users are not. Is that an acceptable divergence, or should ICU
go to var_cppflags as well?
Btw the fake .pc test idea sounds good as a follow-up! No need to block v3 on it from my side.
Thanks again for picking this up!
Kind regards,
Yuriy
On Sep 30, 2026, at 07:53, Grigorev Jurij <ju.grigorev@ftdata.ru> wrote:
Hi Aidar,
Thanks for the quick update. v2 looks much closer to me!
I like that both pkgconfig and cmake are covered now, and that all include dirs are kept.
Placement still looks right too: OpenSSL in CPPFLAGS and ICU in ICU_CFLAGS,
matching Makefile.global and autoconf.Three small worries, all about cases where flags end up silently empty:
First, the hand-rolled pkg-config call:
if ssl.type_name() == 'pkgconfig' and pkg_config.found()
openssl_cflags = run_command(pkg_config, '--cflags', 'openssl',
check: false).stdout().strip()Could we check returncode() here, use native: true, and query "icu-uc icu-i18n" together like configure does?
Right now a failed pkg-config just gives empty flags, and the extension fails much later with a missing header.Second, pkg_config is defined in one file but used in another:
src/include/meson.build:
pkg_config = find_program( 'pkg-config', required: false)src/makefiles/meson.build:
if icu.type_name() == 'pkgconfig' and pkg_config.found()It works today because of subdir ordering, but it is fragile.
Maybe move it up or look it up locally?Third, the cmake branch:
foreach d : ssl.get_variable(cmake: 'OPENSSL_INCLUDE_DIR',
default_value: '').split(';')Are OPENSSL_INCLUDE_DIR and ICU_INCLUDE_DIRS valid for
both Find-modules and upstream config files? With default_value: ''
a wrong name just means empty flags. Same for find_library deps --
I agree extra_include_dirs is a separate gap, but a short comment would help.One question, not a blocker: with this split, pg_config --cppflags still misses ICU,
unlike autoconf. PGXS make is fine through $(ICU_CFLAGS), but direct
pg_config users are not. Is that an acceptable divergence, or should ICU
go to var_cppflags as well?Btw the fake .pc test idea sounds good as a follow-up! No need to block v3 on it from my side.
Thanks again for picking this up!Kind regards,
Yuriy
Hi Yuriy,
Thanks for the careful review. v3 attached.
Could we check returncode(), use native: true, and query
"icu-uc icu-i18n" together like configure does?
returncode() is checked now, and a failure is a hard error carrying
pkg-config's stderr, same as python_includespec effectively does. ICU is
queried as "icu-uc icu-i18n" in one call. While there I made the call
mirror Meson's own pkg-config invocation: it sets PKG_CONFIG_PATH from
the pkg_config_path option, and PKG_CONFIG_SYSROOT_DIR /
PKG_CONFIG_LIBDIR from the sys_root / pkg_config_libdir properties.
v2 silently used the shell environment instead, so with
-Dpkg_config_path=... it could query a different .pc than Meson used.
This also covers the sysroot concern from your first review.
I did not use native: true, though. Meson looks up pkg-config for host
dependencies on the host machine (cross-file entry, then PKG_CONFIG,
then PATH). native: true would skip the cross-file entry and query the
build machine's .pc files. The default find_program() lookup is the
same one Meson uses.
pkg_config is defined in one file but used in another
All flag extraction now lives in one block in the top-level meson.build,
after all dependencies are resolved. src/include and src/makefiles only
consume the result.
Are OPENSSL_INCLUDE_DIR and ICU_INCLUDE_DIRS valid for both
Find-modules and upstream config files?
Good catch, and the answer was "no" for OpenSSL. Meson's CMake trace
only records set() calls, and FindOpenSSL assigns OPENSSL_INCLUDE_DIR
via find_path() only, so in the default module mode v2 always produced
empty flags. Only OpenSSL's upstream OpenSSLConfig.cmake set()s it.
v3 uses PACKAGE_INCLUDE_DIRS instead, which Meson's CMake wrapper
derives from <Name>_INCLUDE_DIRS / _INCLUDES / _INCLUDE_DIR. I checked
it against FindOpenSSL, OpenSSLConfig.cmake and FindICU (ICU ships no
config files). It's internal to Meson, but has been there since at
least 0.57.2. If it ever comes up empty, setup emits a warning instead
of silently producing nothing. A comment now explains the 'system'
method (headers already on the default path) and the find_library /
extra_include_dirs gap.
should ICU go to var_cppflags as well?
pg_config --cppflags now includes ICU, for parity with autoconf. It's
added to VAL_CPPFLAGS only, not Makefile.global's CPPFLAGS, so PGXS
doesn't get it twice. One note: since af2d4ca191a no installed header
includes ICU anymore, so on master the ICU part is about autoconf
parity and extensions using ICU themselves.
Regards,
Aidar Imamov
On Oct 14, 2026, at 18:08, Aidar Imamov <imamovaj22@gmail.com> wrote:
On Sep 30, 2026, at 07:53, Grigorev Jurij <ju.grigorev@ftdata.ru> wrote:
Hi Aidar,
Thanks for the quick update. v2 looks much closer to me!
I like that both pkgconfig and cmake are covered now, and that all include dirs are kept.
Placement still looks right too: OpenSSL in CPPFLAGS and ICU in ICU_CFLAGS,
matching Makefile.global and autoconf.Three small worries, all about cases where flags end up silently empty:
First, the hand-rolled pkg-config call:
if ssl.type_name() == 'pkgconfig' and pkg_config.found()
openssl_cflags = run_command(pkg_config, '--cflags', 'openssl',
check: false).stdout().strip()Could we check returncode() here, use native: true, and query "icu-uc icu-i18n" together like configure does?
Right now a failed pkg-config just gives empty flags, and the extension fails much later with a missing header.Second, pkg_config is defined in one file but used in another:
src/include/meson.build:
pkg_config = find_program( 'pkg-config', required: false)src/makefiles/meson.build:
if icu.type_name() == 'pkgconfig' and pkg_config.found()It works today because of subdir ordering, but it is fragile.
Maybe move it up or look it up locally?Third, the cmake branch:
foreach d : ssl.get_variable(cmake: 'OPENSSL_INCLUDE_DIR',
default_value: '').split(';')Are OPENSSL_INCLUDE_DIR and ICU_INCLUDE_DIRS valid for
both Find-modules and upstream config files? With default_value: ''
a wrong name just means empty flags. Same for find_library deps --
I agree extra_include_dirs is a separate gap, but a short comment would help.One question, not a blocker: with this split, pg_config --cppflags still misses ICU,
unlike autoconf. PGXS make is fine through $(ICU_CFLAGS), but direct
pg_config users are not. Is that an acceptable divergence, or should ICU
go to var_cppflags as well?Btw the fake .pc test idea sounds good as a follow-up! No need to block v3 on it from my side.
Thanks again for picking this up!Kind regards,
YuriyHi Yuriy,
Thanks for the careful review. v3 attached.
Could we check returncode(), use native: true, and query
"icu-uc icu-i18n" together like configure does?returncode() is checked now, and a failure is a hard error carrying
pkg-config's stderr, same as python_includespec effectively does. ICU is
queried as "icu-uc icu-i18n" in one call. While there I made the call
mirror Meson's own pkg-config invocation: it sets PKG_CONFIG_PATH from
the pkg_config_path option, and PKG_CONFIG_SYSROOT_DIR /
PKG_CONFIG_LIBDIR from the sys_root / pkg_config_libdir properties.
v2 silently used the shell environment instead, so with
-Dpkg_config_path=... it could query a different .pc than Meson used.
This also covers the sysroot concern from your first review.I did not use native: true, though. Meson looks up pkg-config for host
dependencies on the host machine (cross-file entry, then PKG_CONFIG,
then PATH). native: true would skip the cross-file entry and query the
build machine's .pc files. The default find_program() lookup is the
same one Meson uses.pkg_config is defined in one file but used in another
All flag extraction now lives in one block in the top-level meson.build,
after all dependencies are resolved. src/include and src/makefiles only
consume the result.Are OPENSSL_INCLUDE_DIR and ICU_INCLUDE_DIRS valid for both
Find-modules and upstream config files?Good catch, and the answer was "no" for OpenSSL. Meson's CMake trace
only records set() calls, and FindOpenSSL assigns OPENSSL_INCLUDE_DIR
via find_path() only, so in the default module mode v2 always produced
empty flags. Only OpenSSL's upstream OpenSSLConfig.cmake set()s it.
v3 uses PACKAGE_INCLUDE_DIRS instead, which Meson's CMake wrapper
derives from <Name>_INCLUDE_DIRS / _INCLUDES / _INCLUDE_DIR. I checked
it against FindOpenSSL, OpenSSLConfig.cmake and FindICU (ICU ships no
config files). It's internal to Meson, but has been there since at
least 0.57.2. If it ever comes up empty, setup emits a warning instead
of silently producing nothing. A comment now explains the 'system'
method (headers already on the default path) and the find_library /
extra_include_dirs gap.should ICU go to var_cppflags as well?
pg_config --cppflags now includes ICU, for parity with autoconf. It's
added to VAL_CPPFLAGS only, not Makefile.global's CPPFLAGS, so PGXS
doesn't get it twice. One note: since af2d4ca191a no installed header
includes ICU anymore, so on master the ICU part is about autoconf
parity and extensions using ICU themselves.Regards,
Aidar Imamov<v3-0001-meson-Expose-OpenSSL-and-ICU-compiler-flags-to-PG.patch>
Hi again,
v4 attached. It fixes the configure failure CFBot reported for v3
("environment.set takes at least 2 arguments, but got 1"). I passed
get_option('pkg_config_path') to environment.set() unconditionally,
and that option is empty when PKG_CONFIG_PATH is not set, which is why
it worked in my shell but not in CI. It is now only set when
non-empty, like the sys_root / pkg_config_libdir handling next to it.
Verified with Meson 1.7.0 and 0.57.2 (current min), with and without
PKG_CONFIG_PATH, and with the configure options CFBot uses.
Apart from that, v4 only makes the new block easier to read: the list
of dependencies is now a named table with named fields ('deps',
'pc_modules') instead of positional lists, and the comments are
shorter. Nothing else changed from v3.
Since v3 looked quite different from v2, a word on why the code moved
into one block in the top-level meson.build instead of staying split
between src/include and src/makefiles:
- With ICU added to pg_config --cppflags, the ICU flags are needed
when VAL_CPPFLAGS is built in src/common, which is processed before
src/makefiles. So the ICU part could not stay where v2 had it.
- With the returncode() check, pkg-config has to see the same
PKG_CONFIG_PATH as Meson. Otherwise a build configured with only
-Dpkg_config_path would now fail at setup, where v2 silently
produced empty flags. That environment setup is shared by OpenSSL
and ICU, as is the pkg_config lookup you asked to move, so keeping
everything in one place avoids duplicating it.
The approach itself is unchanged from v2: ask pkg-config or CMake for
the flags, put OpenSSL into CPPFLAGS and ICU into ICU_CFLAGS. Adding
another dependency is one more entry in the table.
Regards,
Aidar Imamov
Both follow-up patches are attached in order on top of your public v4.
Patch 1 handles CMake include directories returned as strings or lists
and escapes spaces in exported flags. Patch 2 keeps PostgreSQL headers
ahead of extra include paths, passes those paths to all six fallback
header checks, and exports them through PGXS and pg_config.
I applied v4 and these exact patches to PostgreSQL master
6b9f40d49ae33eb0b4a7c77666fe4971ef7ba087. On hosted Windows x64 and
ARM64, the include-export cases passed with Meson 0.57.2 and 1.11.2.
All six fallback header probes and the psql header-order check passed
on both architectures. The v4 control reproduced the wrong-header
failure.
Hosted run: https://github.com/Harrison-Booth/postgresql-windows-ci/actions/runs/39595041637
Patch 2 addresses the same header-order concern as CF6056, but also
covers fallback probes and exported flags. I haven't combined or
tested both implementations together. These focused checks don't cover
a full server, installed extension, or PGXS runtime.
Could you review the attached follow-up patches, especially patch 2's
header-order approach?
Best,
Harrison
Attachments:
0001-Meson-handle-CMake-include-directory-lists-and-space.patchtext/x-patch; charset=US-ASCII; name=0001-Meson-handle-CMake-include-directory-lists-and-space.patchDownload+6-4
0002-Keep-external-include-flags-after-PostgreSQL-headers.patchtext/x-patch; charset=US-ASCII; name=0002-Keep-external-include-flags-after-PostgreSQL-headers.patchDownload+42-12
Hi,
Thanks a lot for looking into this, and for the Windows runs, that's
exactly the coverage I couldn't get myself.
I've split things up. v5 attached is just my v4 plus your first patch,
squashed. Your second patch I'd rather keep separate for now, more on
that below, but I did review it properly and tried it on top of v5.
About your patch 1. Handling PACKAGE_INCLUDE_DIRS being either a string
or an array makes sense, and escaping spaces the same way pkg-config
does keeps the output consistent with the pkg-config branch. I changed
it a little: empty entries are now skipped before the -I flag is built
rather than after, and the string/array normalization is done with
';'.join([] + incdirs) instead of the temporary list. Behaviour should
be the same. I have to admit I couldn't exercise the CMake branch at
all here, since on macOS Homebrew's OpenSSL and ICU are found through
pkg-config, so as far as I can tell your Windows runs are the only
real coverage of that path so far. If you have a minute to rerun them
with v5, that would be great.
You mentioned that your checks didn't cover an installed extension or
the PGXS runtime, so I did that part. I built and installed master +
v5 with -Dssl=openssl -Dicu=enabled, then built `credcheck` extension through PGXS
against the installed tree. pg_config --cppflags and Makefile.global
now carry the Homebrew OpenSSL and ICU include dirs, the extension
compiles. For a control I blanked CPPFLAGS and
ICU_CFLAGS in the installed Makefile.global, which is what an
unpatched build produces, and the same build fails right away on
libpq/libpq-be.h:25 with 'openssl/ssl.h' file not found.
Now your patch 2. The approach itself looks right to me. I checked the
compile commands and the in-tree dirs (src/include, libpq's dirs) now
always come before extra_include_dirs. Passing header_args to
the six find_library() probes is needed too, since those only got
postgres_inc and project arguments aren't applied to compiler checks.
A few things I noticed while going through it:
1. There's a seventh probe that lost the extra dirs: the Python.h
check (the one marked "remove after we depend on Meson >= 1.1.0")
used include_directories: postgres_inc, so after the change it no
longer sees extra_include_dirs. I went through the other checks
too. numa.h, tcl.h, the curl run test and ngettext never had
postgres_inc or test_c_args, so nothing changes for them.
2. The backslash-to-slash conversion runs on every platform. I guess
it's there to keep Windows backslashes out of the C string literal
in config_info.c and out of make, which makes sense, but on Linux
or macOS a backslash is a legal character in a directory name. I
limited it to host_system == 'windows'.
3. This one is more of a question than a bug. Since project arguments
come after dependency compile args, the include dirs from
pkg-config and CMake dependencies now take precedence over
extra_include_dirs. Before it was the other way around. For
example, be-secure-openssl.c on master gets
src/include, <extra dirs>, icu, lz4, openssl, zstd
and with your patch
src/include, icu, lz4, openssl, zstd, <extra dirs>
autoconf behaves like master here, --with-includes ends up before
the dependency flags. So if someone uses extra_include_dirs to
point at their own OpenSSL headers, and some other dependency's
.pc file happens to add a dir that also has OpenSSL headers (say
/usr/local/include), they'd now quietly get the other copy. It's a
corner case and probably an acceptable price for fixing the libpq
ordering, but it should be a conscious decision. I'd guess Andres's
prototype in CF6056 has the same property. Do you know if that was
discussed there?
4. A relative path in extra_include_dirs used to be resolved by
include_directories() against the source dir. As a plain -I it's
now relative to the build dir, since that's where ninja runs the
compiler. include_directories() also refused nonexistent dirs,
while a plain -I just gets ignored, which is how autoconf behaves
anyway. The docs show absolute paths, so it probably doesn't
matter much, but it's worth mentioning in the commit message.
5. Escaping only spaces is fine by me, it's what pkg-config does. As a
check I took the CPPFLAGS line from a generated Makefile.global
with a dir containing a space and compiled through make, and the
header was found. Something like cc $(pg_config --cppflags) in a
shell will still split on the space, but that's equally true of
pkg-config output and of autoconf builds.
6. Pre-existing, but since we're here: the LLVM bitcode flags in
src/backend/jit/llvm/meson.build never got extra_include_dirs,
before or after this change. That's a separate fix though.
Why I kept patch 2 out of v5: it changes how the tree itself is built,
not just what we export to PGXS, and it overlaps CF6056, where Thomas
and Andres already have opinions on this exact ordering. I'd rather
not tie the PGXS fix to that discussion, because v5 on its own is
small and fixes a real build failure for extensions.
Mb you can take your 2-patch proposal to the CF6056 thread, if you prefer.
It would also be good to hear from Thomas or Andres whether they'd rather
have it folded into their patch.
Thanks again,
Aidar
v6 attached.
I left your patch 1 as it was, apart from a comment.
I had first simplified the string/array handling to ';'.join([] + incdirs), but
Meson 0.57.2 rejects list + string with plain +, only += accepts it,
so your two-line version is the one that works. I caught that by
forcing the CMake fallback on both 0.57.2 and 1.7: both now produce the
right -I flags.
Regards,
Aidar Imamov