Re: Improve error message for ICU libraries if pkg-config is absent - Mailing list pgsql-hackers

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