| From: | Aidar Imamov <imamovaj22(at)gmail(dot)com> |
|---|---|
| To: | Grigorev Jurij <ju(dot)grigorev(at)ftdata(dot)ru> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: meson: pass OpenSSL/ICU include dirs to extensions |
| Date: | 2026-10-14 17:11:38 |
| Message-ID: | D7443D30-8CBD-47F0-A45B-54EC015679E5@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Oct 14, 2026, at 18:08, Aidar Imamov <imamovaj22(at)gmail(dot)com> wrote:
>
>
>
>> On Sep 30, 2026, at 07:53, Grigorev Jurij <ju(dot)grigorev(at)ftdata(dot)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
>
> <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
| Attachment | Content-Type | Size |
|---|---|---|
| v4-0001-meson-expose-OpenSSL-and-ICU-compiler-flags-to-PG.patch | application/octet-stream | 7.3 KB |
| unknown_filename | text/plain | 1 byte |
| From | Date | Subject | |
|---|---|---|---|
| Previous Message | Zsolt Parragi | 2026-10-14 16:50:41 | Re: pgstat: allow a stats kind to use its own dedicated dsa/dshash |