iconv is not used in Windows builds, so don't require in configure

dbry / WavPack

WavPack encode/decode library, command-line programs, and several plugins

BSD 3-Clause "New" or "Revised" License

346 stars 65 forks source link

iconv is not used in Windows builds, so don't require in configure #105

Closed dbry closed 3 years ago

dbry commented 3 years ago

@SoapGentoo I found a very minor issue with the new configure script where it would require iconv even for Windows builds (which use a native Windows API instead). This patch removes that dependency but I wanted to run it by you because I have no idea what I'm doing. Thanks.

SoapGentoo commented 3 years ago

yes looks good!

dbry commented 3 years ago

Thanks!

Related to this, I just received notice that the Google Fuzz build is failing. Here's the log:

https://oss-fuzz-build-logs.storage.googleapis.com/log-7ef97b53-e5ff-42b3-b888-0fedba144743.txt

It seems to be that AM_ICONV is missing (we used to not actually use that macro, but implemented it ourselves in configure.ac).

Will this error be fixed if we just add gettext to the packages? I see that it's mentioned as suggested in the logs, but not brought it.

SoapGentoo commented 3 years ago

Yes, /usr/share/aclocal/iconv.m4 is part of gettext on my system and contains the definition. Would it be hard adding this dependency? In general, hiding this stuff in gettext is nicer, as the detection code for the iconv API is super brittle and gettext's implementation is battle tested.