Link issue on macOS - branch/next

[npm-sdr]

Hi,

Latest dablin_gtk on branch/next link against libiconv but no explicit entry in src/CMakeLists.txt to link libiconv. This causes dablin_gtk link to fail on macOS.

I've attached a patch below, assuming this src/CMakeLists.txt is the right place to add the lib.

Regards, Niro.

dablin_iconv.patch.txt

[basicmaster]

Hi Niro,

thanks for pointing this out.

Just adding iconv to the linker libs unfortunately doesn't work, as there are two cases here:

• glibc with its built-in iconv implementation is used, so adding iconv to the linker libs is not required/allowed
• libiconv is used and it therefore requires to add iconv to the linker libs

CMake already has a module to abstract from this case, but it was only added two months ago and will not be available for most of the users.

So I added a simpler check that uses libiconv if available and otherwise assumes that an internal implementation is available. The glibc case works for me - could you please check if the libiconv case works for you?

[npm-sdr]

Hi,

Unfortunately, this didn't work because pkg_check_modules(ICONV iconv) doesn't find macOS xcode's preinstalled iconv. This would have worked if an alternative version of libiconv had been installed from a package manager (e.g. brew/mac-ports).

Having googled cmake iconv, I found the attached works for both Linux and macOS.

Regards, Niro.

dablin_iconv_patch2.txt

[basicmaster]

Hi Niro,

OK, I see. So it seems that we have to search by hand for it.

Going through the code, it turns out that the code does some unnecessary things (e.g. CheckCCompilerFlag is included, but never used), and you said that you used Google...so is this a solution you found during your search (and which just worked), or did you create this on your own (to consider all possible cases)?

[npm-sdr]

Hi Stefan,

I've removed unnecessary inclusions (CheckCCompilerFlag and CheckCSourceCompiles). This was based on what I found during Google search. I found quite a few occurrences of FindICONV.cmake files in other projects, including the change made to cmake itself. I based mine on https://github.com/Kitware/CMake/blob/release/Modules/FindIconv.cmake but I haven't considered all possible cases. I just tested on Linux 3.19.0-84-generic (Ubuntu 15.04) and 3.2.0-92-generic-pae (Ubuntu 12.04.5 LTS).

FindIconv module is on cmake git-master but not made to a release yet. Latest release version of cmake I can get from the package manager on macOS is 3.10.2 but FindIconv is not in this release.

It's not great having to put a short-term workaround until the next release of cmake. Perhaps we could ask cmake maintainers to do a release? My patch could stay in the 'next' branch until then and I'm happy take it out when it can be taken out.

Regards, Niro.

[basicmaster]

Hi Niro,

waiting for CMake to make a release is not really an option, as this would require every user to upgrade to it in addition.

Reading the current patch I'm unfortunately still not really convinced, as it contains that part with CMAKE_C_FLAGS_BACKUP without real need; so it is still not really optimal.

Looking for other solutions, I also found a CMake module from LyX which seems to work with older versions; at least it works for me.

Also looking at the already present CMake modules added by @mnhauke, another option would be to just copy from the FAAD module and change the var names as well as the possible names of header/lib: iconv.patch.txt

What do you think about all the possible solutions and how do the two others work for you/on your Mac?

[npm-sdr]

Hi Stefan,

This patch works for macOS and there's only one flavour of it so the test space is very tiny. If you're are sure about all Linux variations, which I am, then all good from my side.

Regards, Niro.

PS: I haven't tried on Ubuntu 10.04 but I can't see why anyone should run this on such an old distro.

[basicmaster]

OK, great!

10.04 shouldn't work, as already 12.04 needs some manual changes (due to C++11 differences) to work. 14.04 should probably be fine and 16.04 definitely is.

So I added the patch to the repo and therefore close here.