PR#50111: Issue 50067: Use pkg-config to detect icu - 389-ds-base

389-ds-base

#50111 Issue 50067: Use pkg-config to detect icu

Closed 3 years ago by spichugi. Opened 5 years ago by hmc.

hmc/389-ds-base icu-i18n into master

Ticket 50111: Use pkg-config to detect icu

Hugh McMaster • 5 years ago

24271fe

Makefile.am

file modified

+4 -5

		`@@ -172,7 +172,6 @@`

		`DB_LINK = @db_lib@ -ldb-@db_libver@`
		`SASL_LINK = @sasl_lib@ -lsasl2`
		`- ICU_LINK = @icu_lib@ -licui18n -licuuc -licudata`
		`PCRE_LINK = @pcre_lib@ -lpcre`
		`NETSNMP_LINK = @netsnmp_lib@ @netsnmp_link@`
		`PAM_LINK = -lpam`
		`@@ -1614,8 +1613,8 @@`
		`ldap/servers/plugins/collation/config.c \`
		`ldap/servers/plugins/collation/orfilter.c`

		`- libcollation_plugin_la_CPPFLAGS = $(AM_CPPFLAGS) $(DSPLUGIN_CPPFLAGS) @icu_inc@`
		`- libcollation_plugin_la_LIBADD = libslapd.la $(LDAPSDK_LINK) $(NSPR_LINK) $(ICU_LINK) $(LIBCSTD) $(LIBCRUN)`
		`+ libcollation_plugin_la_CPPFLAGS = $(AM_CPPFLAGS) $(DSPLUGIN_CPPFLAGS) $(ICU_CFLAGS)`
		`+ libcollation_plugin_la_LIBADD = libslapd.la $(LDAPSDK_LINK) $(NSPR_LINK) $(ICU_LIBS) $(LIBCSTD) $(LIBCRUN)`
		`libcollation_plugin_la_DEPENDENCIES = libslapd.la`
		`libcollation_plugin_la_LDFLAGS = -avoid-version`
		`# libcollation_plugin_la_LINK = $(CXXLINK) -avoid-version`
		`@@ -1857,8 +1856,8 @@`
		`ldap/servers/plugins/replication/windows_protocol_util.c \`
		`ldap/servers/plugins/replication/windows_tot_protocol.c`

		`- libreplication_plugin_la_CPPFLAGS = $(AM_CPPFLAGS) $(DSPLUGIN_CPPFLAGS) @icu_inc@ @db_inc@`
		`- libreplication_plugin_la_LIBADD = libslapd.la $(LDAPSDK_LINK) $(NSS_LINK) $(NSPR_LINK) $(ICU_LINK) $(DB_LINK)`
		`+ libreplication_plugin_la_CPPFLAGS = $(AM_CPPFLAGS) $(DSPLUGIN_CPPFLAGS) $(ICU_CFLAGS) @db_inc@`
		`+ libreplication_plugin_la_LIBADD = libslapd.la $(LDAPSDK_LINK) $(NSS_LINK) $(NSPR_LINK) $(ICU_LIBS) $(DB_LINK)`
		`libreplication_plugin_la_DEPENDENCIES = libslapd.la`
		`libreplication_plugin_la_LDFLAGS = -avoid-version`

configure.ac

file modified

+4 -1

		`@@ -34,6 +34,7 @@`
		`AM_PROG_CC_C_O`
		`AM_PROG_AS`
		`AC_PROG_CC_STDC`
		`+ PKG_PROG_PKG_CONFIG`

		`# disable static libs by default - we only use a couple`
		`AC_DISABLE_STATIC`
		`@@ -822,7 +823,9 @@`
		`m4_include(m4/mozldap.m4)`
		`m4_include(m4/db.m4)`
		`m4_include(m4/sasl.m4)`
		`- m4_include(m4/icu.m4)`
		`+`
		`+ PKG_CHECK_MODULES([ICU], [icu-i18n >= 60.2])`
		`+`
		`m4_include(m4/netsnmp.m4)`
		`m4_include(m4/kerberos.m4)`
		`m4_include(m4/pcre.m4)`

m4/icu.m4

file removed

-100

		`@@ -1,100 +0,0 @@`
		`- # BEGIN COPYRIGHT BLOCK`
		`- # Copyright (C) 2006 Red Hat, Inc.`
		`- # All rights reserved.`
		`- #`
		`- # License: GPL (version 3 or any later version).`
		`- # See LICENSE for details.`
		`- # END COPYRIGHT BLOCK`
		`-`
		`- AC_CHECKING(for LIBICU)`
		`-`
		`- # check for --with-icu`
		`- AC_MSG_CHECKING(for --with-icu)`
		`- AC_ARG_WITH(icu, AS_HELP_STRING([--with-icu@<:@=PATH@:>@],[ICU directory]),`
		`- [`
		`- if test "$withval" = "yes"`
		`- then`
		`- AC_MSG_RESULT(yes)`
		`- elif test "$withval" = "no"`
		`- then`
		`- AC_MSG_RESULT(no)`
		`- AC_MSG_ERROR([ICU is required.])`
		`- elif test -d "$withval"/lib`
		`- then`
		`- AC_MSG_RESULT([using $withval])`
		`- ICUDIR=$withval`
		`- icu_lib="-L$ICUDIR/lib"`
		`- icu_inc="-I$withval/include"`
		`- icu_bin="$withval/bin"`
		`- else`
		`- echo`
		`- AC_MSG_ERROR([$withval not found])`
		`- fi`
		`- ],`
		`- AC_MSG_RESULT(yes))`
		`-`
		`- # check for --with-icu-inc`
		`- AC_MSG_CHECKING(for --with-icu-inc)`
		`- AC_ARG_WITH(icu-inc, AS_HELP_STRING([--with-icu-inc=PATH],[ICU include directory]),`
		`- [`
		`- if test -d "$withval"`
		`- then`
		`- AC_MSG_RESULT([using $withval])`
		`- icu_inc="-I$withval"`
		`- else`
		`- echo`
		`- AC_MSG_ERROR([$withval not found])`
		`- fi`
		`- ],`
		`- AC_MSG_RESULT(no))`
		`-`
		`- # check for --with-icu-lib`
		`- AC_MSG_CHECKING(for --with-icu-lib)`
		`- AC_ARG_WITH(icu-lib, AS_HELP_STRING([--with-icu-lib=PATH],[ICU library directory]),`
		`- [`
		`- if test -d "$withval"`
		`- then`
		`- AC_MSG_RESULT([using $withval])`
		`- icu_lib="-L$withval"`
		`- else`
		`- echo`
		`- AC_MSG_ERROR([$withval not found])`
		`- fi`
		`- ],`
		`- AC_MSG_RESULT(no))`
		`-`
		`- # check for --with-icu-bin`
		`- AC_MSG_CHECKING(for --with-icu-bin)`
		`- AC_ARG_WITH(icu-bin, AS_HELP_STRING([--with-icu-bin=PATH],[ICU binary directory]),`
		`- [`
		`- if test -d "$withval"`
		`- then`
		`- AC_MSG_RESULT([using $withval])`
		`- icu_bin="$withval"`
		`- else`
		`- echo`
		`- AC_MSG_ERROR([$withval not found])`
		`- fi`
		`- ],`
		`- AC_MSG_RESULT(no))`
		`- # if ICU is not found yet, try pkg-config`
		`-`
		`- # last resort`
		`- if test -z "$icu_lib"; then`
		`- AC_PATH_PROG(ICU_CONFIG, icu-config)`
		`- AC_MSG_CHECKING(for icu with icu-config)`
		`- if test -n "$ICU_CONFIG"; then`
		- icu_lib=`$ICU_CONFIG --ldflags-searchpath`
		- icu_inc=`$ICU_CONFIG --cppflags-searchpath`
		- icu_bin=`$ICU_CONFIG --bindir`
		`- AC_MSG_RESULT([using system ICU])`
		`- else`
		`- AC_MSG_ERROR([ICU not found, specify with --with-icu.])`
		`- fi`
		`- fi`
		`-`
		`-`
		`- AC_SUBST(icu_lib)`
		`- AC_SUBST(icu_inc)`
		`- AC_SUBST(icu_bin)`
		`-`

hmc commented 5 years ago

This patch alters the build system to use pkg-config to detect icu.

hmc commented 5 years ago

Hi there,

Any progress on reviewing this PR?

Debian is about to remove icu-config from its libicu-dev package. Ubuntu and Linux Mint will follow soon after.

Thank you

Edited 5 years ago by hmc

mreynolds commented 5 years ago

Hi there,
Any progress on reviewing this PR?
Debian is about to remove icu-config from its libicu-dev package. Ubuntu and Linux Mint will follow soon after.
Thank you\

I'll review and test this today...

mreynolds commented 5 years ago

Sorry this patch breaks the server on Fedora/RHEL. Server fails to start:

[23/Jan/2019:10:06:58.503357687 -0500] - ERR - symload_report_error - Netscape Portable Runtime error -5977: /usr/lib64/dirsrv/plugins/libcollation-plugin.so: undefined symbol: ucol_getSortKey_60
[23/Jan/2019:10:06:58.505590181 -0500] - ERR - symload_report_error - Could not open library "/usr/lib64/dirsrv/plugins/libcollation-plugin.so" for plugin Internationalization Plugin
[23/Jan/2019:10:06:58.507994436 -0500] - ERR - slapd_bootstrap_config - The plugin entry [cn=Internationalization Plugin,cn=plugins,cn=config] in the configfile /etc/dirsrv/slapd-localhost/dse.ldif was invalid. Failed to load plugin's init function.
[23/Jan/2019:10:06:58.510315403 -0500] - EMERG - main - The configuration files in directory /etc/dirsrv/slapd-localhost could not be read or were not found.  Please refer to the error log or output for more information.

firstyear commented on line 86 of m4/icu.m4 5 years ago

You add enable_icu (which is fine), but you need icu_lib, icu_inc and icu_bin, as well ass the AC_SUBST below. You might want to check how we use pkg-config in one of the other .m4 files to fill in these values, but I think the missing icu_* and AC_SUBST causes the issue that @mreynolds noticed.

hmc commented 5 years ago

@mreynolds, @firstyear Thank you for your feedback. I'll have a close look at this, as the issue is not evident on Debian.

Also note that icu_bin is not used anywhere at all in the original codebase. I'm also fairly certain that the other icu_* variables are adequately covered by the patch. But it's been a while, so I'll check now.

hmc commented 5 years ago

@mreynolds Did you pass --enable-icu to configure?

Can you please also post the output of:
pkg-config --modversion icu-i18n
icu-config --cppflags-searchpath
pkg-config --cflags icu-i18n
icu-config --ldflags-searchpath
pkg-config --libs icu-i18n

On Debian, these are the just the default environment paths (e.g. -I/usr/include and -L/usr/lib/x86_64-linux-gnu). pkg-config doesn't output anything, since those paths are included/searched by default.

mreynolds commented 5 years ago

@mreynolds Did you pass --enable-icu to configure?

I did not, so you should add that to the 389-ds-base/rpm/389-ds-base.spec.in file since it's apparently required

Can you please also post the output of:
pkg-config --modversion icu-i18n
icu-config --cppflags-searchpath
pkg-config --cflags icu-i18n
icu-config --ldflags-searchpath
pkg-config --libs icu-i18n

Here it is

[root@localhost cli]# pkg-config --modversion icu-i18n
60.2
[root@localhost cli]# icu-config --cppflags-searchpath
-I/usr/include 
[root@localhost cli]# pkg-config --cflags icu-i18n

[root@localhost cli]# icu-config --ldflags-searchpath
-L/usr/lib64 
[root@localhost cli]# pkg-config --libs icu-i18n
-licui18n -licuuc -licudata

Edited 5 years ago by mreynolds

firstyear commented 5 years ago

@hmc Thanks for that, I'm currently away from the office, but I'll have a more detailed review monday next week. Appreciate your contribution and time :)

firstyear commented 5 years ago

We probably want enable-icu by default rather than needing to explicitly request it (IE if you DONT want it you have to pass "--disable-icu" instead.

hmc commented 5 years ago

Thanks for posting the output, @mreynolds. That confirms icu is installed in a standard system path.

Moving forward, I agree we should have icu enabled by default. We should also probably set a minimum version for icu, say 60.2.

In terms of coding, PKG_CHECK_MODULES can take care of this version check. Or, if you want to use manual calls to pkg-config (e.g. passing --cflags-only-I and --libs-only-L) for consistency with other m4 modules, we can do this too. For icu, the output is the same in both methods, since pkg-config strips the system paths by default and there are no defines.

Personally, I prefer PKG_CHECK_MODULES, since it is simpler and calls AC_SUBST for <pkg>_CFLAGS and <pkg>_LIBS. It also allows users to override those variables at configure-time.

Edited 5 years ago by hmc

hmc commented 5 years ago

@mreynolds, @firstyear:

Do you have a preference on a way forwards?

Once I know, I’ll rework the patch.

Thank you

firstyear commented 5 years ago

@hmc I'd be happy to see what a patch with PKG_CHECK_MODULES looks like, with the version check. We are pretty open to improvements and new ideas so I think go with that. Thanks!

rebased onto fa1637888e828be39e11c766eded7cc36f53e21b

5 years ago

rebased onto 24271fe

5 years ago

hmc commented 5 years ago

I've updated the PR. This version:

Uses PKG_CHECK_MODULES with a version check
Removes m4/icu.m4

Attempting to compile without icu-config and the patch applied results in 389-ds-base failing to build from source, as icu is a build-dependency. My updated PR continues that requirement (unlike the previous version). So, configure will fail if pkg-config cannot find a suitable version of icu installed.

Let me know what you think.

firstyear commented 5 years ago

That is a really elegant and nice change. I really want to update the rest of our modules to use pkg_config like this if possible now!

You have an ack from me, but I'll ask @mreynolds to check as well.

Thanks so much!

hmc commented 5 years ago

It’s certainly possible to do that conversion. If @mreynolds is open for this sort of change, I’ll take a look.

firstyear commented 5 years ago

@hmc Let's just get this one committed first. I'd be happy for you to open a new issue for migrating the rest over if you wanted?

I haven't heard from @mreynolds so I'm wondering if he has been delayed by some other issue? I'm going to merge this (it builds and works for me), and he can tell me off and revert if it's bad.