| 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-09-12 15:44:43 |
| Message-ID: | 8FD3A224-4978-411A-A25A-4566B48AE346@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Sep 9, 2026, at 15:01, Grigorev Jurij <ju(dot)grigorev(at)ftdata(dot)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 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
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
| Attachment | Content-Type | Size |
|---|---|---|
| v2-0001-meson-expose-OpenSSL-and-ICU-compiler-flags-to-PGXS.patch | application/octet-stream | 3.3 KB |
| unknown_filename | text/plain | 2 bytes |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Ayush Tiwari | 2026-09-12 15:50:02 | Re: BUG #19631: currtid2() on a view with GROUP BY ctid crashes with XX000 |
| Previous Message | Ayush Tiwari | 2026-09-12 15:42:10 | Re: Proposal: INSERT ... BY NAME |