Improve error message for ICU libraries if pkg-config is absent
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.
Michael
Meh, forgot the attachment. Also, this should be backpatched to v17 if
accepted.
Michael
Attachments:
0001-Improve-configure-error-for-ICU-libraries-if-pkg-con.patchtext/x-diff; charset=us-asciiDownload
From b696949180437a3c7307ac0509cba54828b44259 Mon Sep 17 00:00:00 2001
From: Michael Banck <michael.banck@credativ.de>
Date: Fri, 9 Aug 2024 10:13:27 +0200
Subject: [PATCH] Improve configure error for ICU libraries if pkg-config is
absent.
If pkg-config is not installed, the ICU libraries cannot be found, but
this was not mentioned in the error message and might lead to confusion
about the actual problem. To improve this, add an additional error
message for the case that pkg-config is not available.
Reported-by: Holger Jakobs
Discussion: https://www.postgresql.org/message-id/ccd579ed-4949-d3de-ab13-9e6456fd2caf%40jakobs.com
---
configure | 7 +++++++
configure.ac | 7 +++++++
2 files changed, 14 insertions(+)
diff --git a/configure b/configure
index 4f3aa44756..b3a2774f1b 100755
--- a/configure
+++ b/configure
@@ -8094,6 +8094,13 @@ $as_echo "$with_icu" >&6; }
if test "$with_icu" = yes; then
+ if test -z "$PKG_CONFIG"; then
+ as_fn_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." "$LINENO" 5
+ fi
pkg_failed=no
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for icu-uc icu-i18n" >&5
diff --git a/configure.ac b/configure.ac
index 049bc01491..18472a464a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -829,6 +829,13 @@ AC_MSG_RESULT([$with_icu])
AC_SUBST(with_icu)
if test "$with_icu" = yes; then
+ if test -z "$PKG_CONFIG"; then
+ AC_MSG_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.])
+ fi
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
--
2.39.2
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?
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?
--
Heikki Linnakangas
Neon (https://neon.tech)
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
Attachments:
v2-0001-Improve-configure-error-for-ICU-libraries-if-pkg-.patchtext/x-diff; charset=us-asciiDownload
From 457bf49bd6bcfb15b8552031ac06e87b5a8b89f7 Mon Sep 17 00:00:00 2001
From: Michael Banck <michael.banck@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
On 09.08.24 10:59, 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?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?
Because we override it with our own message. If we don't supply our own
message, we get the built-in ones. Might be worth trying whether the
built-in ones are better? (But they won't know about higher-level
options like "--without-icu", so they won't be able to give suggestions
like that.)
On Fri, 2024-08-09 at 11:44 +0200, Michael Banck wrote:
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.
That looks good to me. Does anyone have a different opinion? If not,
I'll go ahead and commit (but not backport) this change.
Regards,
Jeff Davis
Hi,
On Wed, Aug 14, 2024 at 06:05:19PM -0700, Jeff Davis wrote:
That looks good to me. Does anyone have a different opinion? If not,
I'll go ahead and commit (but not backport) this change.
What is the rationale not to backpatch this? The error message changes,
but we do not translate configure output, so is this a problem/project
policy to never change configure output in the back-branches?
If the change is too invasive, would something like the initial patch I
suggested (i.e., in the absense of pkg-config, add a more targetted
error message) be acceptable for the back-branches?
Michael
On 15.08.24 09:20, Michael Banck wrote:
On Wed, Aug 14, 2024 at 06:05:19PM -0700, Jeff Davis wrote:
That looks good to me. Does anyone have a different opinion? If not,
I'll go ahead and commit (but not backport) this change.What is the rationale not to backpatch this? The error message changes,
but we do not translate configure output, so is this a problem/project
policy to never change configure output in the back-branches?If the change is too invasive, would something like the initial patch I
suggested (i.e., in the absense of pkg-config, add a more targetted
error message) be acceptable for the back-branches?
But it's not just changing an error message, it's changing the logic
that leads to the error message. Have we really thought through all the
combinations here? I don't know. So committing for master and then
seeing if there is any further feedback seems most prudent.
(I'm not endorsing either patch version here, just commenting on the
process.)