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
v5-0001-meson-expose-OpenSSL-and-ICU-compiler-flags-to-PG.patch
Description: Binary data
