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

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

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.

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.

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

@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 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 

@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 :)

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.

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 _CFLAGS and _LIBS. It also allows users to override those variables at configure-time.

@mreynolds, @firstyear:

Do you have a preference on a way forwards?

Once I know, I’ll rework the patch.

Thank you

@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!

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.

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!

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

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

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3170

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.