Thread: 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
Attachment
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
Attachment
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.)