Hi, adding Jeff to CC as he changed the way ICU configure detection was done in fcb21b3.
On Fri, Aug 09, 2024 at 11:59:12AM +0300, Heikki Linnakangas wrote: > On 09/08/2024 11:16, Michael Banck wrote: > > Hi, > > > > Holger Jacobs complained in pgsql-admin that in v17, if you have the ICU > > development libraries installed but not pkg-config, you get a somewhat > > unhelpful error message about ICU not being present: > > > > |checking for pkg-config... no > > |checking whether to build with ICU support... yes > > |checking for icu-uc icu-i18n... no > > |configure: error: ICU library not found > > |If you have ICU already installed, see config.log for details on the > > |failure. It is possible the compiler isn't looking in the proper > > directory. > > |Use --without-icu to disable ICU support. > > > > The attached patch improves things to that: > > > > |checking for pkg-config... no > > |checking whether to build with ICU support... yes > > |configure: error: ICU library not found > > |The ICU library could not be found because pkg-config is not available, see > > |config.log for details on the failure. If ICU is installed, the variables > > |ICU_CFLAGS and ICU_LIBS can be set explicitly in this case, or use > > |--without-icu to disable ICU support. > > Hmm, if that's a good change, shouldn't we do it for all libraries that we > try to find with pkg-config? Hrm, probably. I think the main difference is that libicu is checked by default (actually since v16, see below), but the others are not, so maybe it is less of a problem there? So I had a further look and the only other pkg-config checks seem to be xml2, lz4 and zstd. From those, lz4 and zstd do not have a custom AC_MSG_ERROR so there you get something more helpful like this: |checking for pkg-config... no [...] |checking whether to build with LZ4 support... yes |checking for liblz4... no |configure: error: in `/home/mbanck/repos/postgres/postgresql/build': |configure: error: The pkg-config script could not be found or is too old. Make sure it |is in your PATH or set the PKG_CONFIG environment variable to the full |path to pkg-config. | |Alternatively, you may set the environment variables LZ4_CFLAGS |and LZ4_LIBS to avoid the need to call pkg-config. |See the pkg-config man page for more details. | |To get pkg-config, see <http://pkg-config.freedesktop.org/>. |See `config.log' for more details The XML check sets the error as no-op because there is xml2-config which is usually used: | if test -z "$XML2_CONFIG" -a -n "$PKG_CONFIG"; then | PKG_CHECK_MODULES(XML2, [libxml-2.0 >= 2.6.23], | [have_libxml2_pkg_config=yes], [# do nothing]) [...] |if test "$with_libxml" = yes ; then | AC_CHECK_LIB(xml2, xmlSaveToBuffer, [], [AC_MSG_ERROR([library 'xml2' (version >= 2.6.23) is required for XML support])]) |fi So if both pkg-config and libxml2-dev are missing this results in: |checking for pkg-config... no [...] |checking whether to build with XML support... yes |checking for xml2-config... no [...] |checking for xmlSaveToBuffer in -lxml2... no |configure: error: library 'xml2' (version >= 2.6.23) is required for XML support Whereas if only pkg-config is missing, configure goes through fine: |checking for pkg-config... no [...] |checking whether to build with XML support... yes |checking for xml2-config... /usr/bin/xml2-config [...] |checking for xmlSaveToBuffer in -lxml2... yes So to summarize, I think the others are fine. > I'm surprised the pkg.m4 module doesn't provide a nice error message already > if pkg-config is not found. I can see some messages like that in pkg.m4. Why > are they not printed? > > > Also, this should be backpatched to v17 if accepted. > Did something change here in v17? I was mistaken, things changed in v16 when ICU was checked for by default and the explicit error message was added. Before, ICU behaved like LZ4/ZST now, i.e. if you added --with-icu explicitly you would get the error about pkg-config not being there. So maybe the better change is to just remove the explicit error message again and depend on PKG_CHECK_MODULES erroring out helpfully? The downside would be that the hint about specifying --without-icu to get around this would disappear, but I think somebody building from source can figure that out more easily than the subtle issue that pkg-config is not installed. This would lead to this: |checking for pkg-config... no |checking whether to build with ICU support... yes |checking for icu-uc icu-i18n... no |configure: error: in `/home/mbanck/repos/postgres/postgresql/build': |configure: error: The pkg-config script could not be found or is too old. Make sure it |is in your PATH or set the PKG_CONFIG environment variable to the full |path to pkg-config. | |Alternatively, you may set the environment variables ICU_CFLAGS |and ICU_LIBS to avoid the need to call pkg-config. |See the pkg-config man page for more details. | |To get pkg-config, see <http://pkg-config.freedesktop.org/>. |See `config.log' for more details I have attached a new patch for that. Additionally, going forward, v18+ could just mandate pkg-config to be installed, but I would assume this is not something we would want to change in the back branches. (I also haven't looked how Meson is handling this) Michael
>From 457bf49bd6bcfb15b8552031ac06e87b5a8b89f7 Mon Sep 17 00:00:00 2001 From: Michael Banck <michael.ba...@credativ.de> Date: Fri, 9 Aug 2024 10:13:27 +0200 Subject: [PATCH v2] Improve configure error for ICU libraries if pkg-config is absent. If pkg-config is not installed, the ICU libraries cannot be found, but the custom configure error message did not mention this. This might lead to confusion about the actual problem. To improve this, remove the explicit error message and rely on PKG_CHECK_MODULES' generic error message. Reported-by: Holger Jakobs Discussion: https://www.postgresql.org/message-id/ccd579ed-4949-d3de-ab13-9e6456fd2caf%40jakobs.com --- configure | 30 ++++++++++++++++++++++-------- configure.ac | 6 +----- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/configure b/configure index 4f3aa44756..14b7ae27e6 100755 --- a/configure +++ b/configure @@ -8153,17 +8153,31 @@ fi # Put the nasty error message in config.log where it belongs echo "$ICU_PKG_ERRORS" >&5 - as_fn_error $? "ICU library not found -If you have ICU already installed, see config.log for details on the -failure. It is possible the compiler isn't looking in the proper directory. -Use --without-icu to disable ICU support." "$LINENO" 5 + as_fn_error $? "Package requirements (icu-uc icu-i18n) were not met: + +$ICU_PKG_ERRORS + +Consider adjusting the PKG_CONFIG_PATH environment variable if you +installed software in a non-standard prefix. + +Alternatively, you may set the environment variables ICU_CFLAGS +and ICU_LIBS to avoid the need to call pkg-config. +See the pkg-config man page for more details." "$LINENO" 5 elif test $pkg_failed = untried; then { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5 $as_echo "no" >&6; } - as_fn_error $? "ICU library not found -If you have ICU already installed, see config.log for details on the -failure. It is possible the compiler isn't looking in the proper directory. -Use --without-icu to disable ICU support." "$LINENO" 5 + { { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5 +$as_echo "$as_me: error: in \`$ac_pwd':" >&2;} +as_fn_error $? "The pkg-config script could not be found or is too old. Make sure it +is in your PATH or set the PKG_CONFIG environment variable to the full +path to pkg-config. + +Alternatively, you may set the environment variables ICU_CFLAGS +and ICU_LIBS to avoid the need to call pkg-config. +See the pkg-config man page for more details. + +To get pkg-config, see <http://pkg-config.freedesktop.org/>. +See \`config.log' for more details" "$LINENO" 5; } else ICU_CFLAGS=$pkg_cv_ICU_CFLAGS ICU_LIBS=$pkg_cv_ICU_LIBS diff --git a/configure.ac b/configure.ac index 049bc01491..94e35da4f2 100644 --- a/configure.ac +++ b/configure.ac @@ -829,11 +829,7 @@ AC_MSG_RESULT([$with_icu]) AC_SUBST(with_icu) if test "$with_icu" = yes; then - PKG_CHECK_MODULES(ICU, icu-uc icu-i18n, [], - [AC_MSG_ERROR([ICU library not found -If you have ICU already installed, see config.log for details on the -failure. It is possible the compiler isn't looking in the proper directory. -Use --without-icu to disable ICU support.])]) + PKG_CHECK_MODULES(ICU, icu-uc icu-i18n) fi # -- 2.39.2
