Subject: Re: Improve error message for ICU libraries if pkg-config is absent

From: Heikki Linnakangas <[email-masked]>
Date: Fri, 9 Aug 2024 11:59:12 +0300

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)

---

From: Michael Banck <[email-masked]>
Date: Fri, 9 Aug 2024 11:44:04 +0200

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:
> 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?
> 

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 <[email-masked]>
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

---

From: Peter Eisentraut <[email-masked]>
Date: Tue, 13 Aug 2024 22:13:27 +0200

> 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.)

---

From: Jeff Davis <[email-masked]>
Date: Wed, 14 Aug 2024 18:05:19 -0700

On Fri, 2024-08-09 at 11:44 +0200, Michael Banck wrote:
> 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

---

From: Michael Banck <[email-masked]>
Date: Thu, 15 Aug 2024 09:20:28 +0200

Hi,

> 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

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 15 Aug 2024 15:22:00 +0200

> 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.)