majn / telegram-purple

Adds support for Telegram to Pidgin, Adium, Finch and other Libpurple based messengers.
GNU General Public License v2.0
735 stars 81 forks source link

configure: fix compilation with clang #553

Closed ConiKost closed 4 years ago

ConiKost commented 4 years ago

If -L/usr/lib is being included, this will break compiling on 64-bit with clang.

Signed-off-by: Conrad Kostecki conrad@kostecki.com

BenWiederhake commented 4 years ago

That's weird, we don't see any issues building with clang, nor does Travis: https://travis-ci.org/github/majn/telegram-purple

./configure is a generated file, so any changes there will be lost.

ConiKost commented 4 years ago

That's weird, we don't see any issues building with clang, nor does Travis: https://travis-ci.org/github/majn/telegram-purple

Does your (CI) system has multilib? The bug only happens, when you have multilib.

For now, I will update your m4_ax_check_zlib.m4 instead. I will see, if I can get somehow in contact with upstream.

BenWiederhake commented 4 years ago

Does your (CI) system has multilib?

I use Debian Testing with the packages binutils-multiarch gcc-9-multilib multiarch-support installed. I think that is a "yes".

The bug only happens, when you have multilib.

Huh, weird.

For now, I will update your m4_ax_check_zlib.m4 instead.

That sounds reasonable, I would like to merge that.

-L$ZLIB_HOME}/lib

That's probably an innocent typo :)

But more generally, I'm a little bit worried that you suggest to never use ZLIB_HOME, even though it was set. This sounds like it's going to break systems whose zlib library lives somewhere else.

How does the following sound? Check whether ZLIB_HOME is just /usr or /usr/, and if so, don't append to LDFLAGS. If the user decides that yes, this is exactly what should happen, they can still just pass ZLIB_HOME=/usr/./ or something, in order to circumvent this. Ugh. This is hack-y, but I don't like the taste of needlessly breaking something.

ConiKost commented 4 years ago

I use Debian Testing with the packages binutils-multiarch gcc-9-multilib multiarch-support installed. I think that is a "yes".

I just looked into that packages, that's not, what I meant by multilib.

Huh, weird.

Let me try to explain, what multilib on Gentoo Linux means. If you install a package (mostly lib stuff) in Gentoo, you have the choice on a 64bit system also to install the 32bit variant.

For example, if I install zlib on my amd64 system and choose to enable 32-bit, I will have at the end: /usr/lib64/libz.so.1 for 64-bit, but also /usr/lib/libz.so.1.

If you are using GCC, the default linker will just warn, that it found incompatible libs in '/usr/lib', which comes from LDFLAGS appended by the m4 stuff. But Clangs default linker is not so friendly and will just abort and says, that the lib is not compatible.

Technically, it's just wrong to pass '/usr/lib' as static folder to the linker. And since zlib is also found by -lz, the path is not needed at all for a default system installation.

How does the following sound? Check whether ZLIB_HOME is just /usr or /usr/, and if so, don't append to LDFLAGS. If the user decides that yes, this is exactly what should happen, they can still just pass ZLIB_HOME=/usr/./ or something, in order to circumvent this.

I can try that.

BenWiederhake commented 4 years ago

you have the choice on a 64bit system also to install the 32bit variant.

For the record: That's precisely what I do on Debian.

ConiKost commented 4 years ago

Please have a look. That change does the job for me.

BenWiederhake commented 4 years ago

I rebased and changed it a little bit (fixed that silly typo, and added a proper warning in the header). See branch dev-1.4.5.

I think this should work, doesn't it? :)

ConiKost commented 4 years ago

Thanks! Lgfm.