[llvm] r316064 - Fix the incorrect detection of ICONV_LIBRARY_PATH

[Eric Beckmann via llvm-commits]

The reason this was added in the first place was that certain builds
(android?  I think) would break when libxml was present but iconv was not.
Libxml2 uses iconv for some internationalization encoding it does (
http://xmlsoft.org/encoding.html).  Even though libxml2 was found on these
builds, a compile time error which went something like “fatal error:
iconv.h: No such file or directory" would be thrown.  This is perhaps a bug
with libxml2 in that it assumes iconv is present in all situations.

I'm not sure why the check was made specific to Linux, perhaps check with
Nico, this was done in r315873.

Sorry this was a while ago and this is most of the info I can give.


On Mon, Nov 6, 2017 at 2:18 PM, Hans Wennborg <hans at chromium.org> wrote:

> I've reverted this in r317517 as it (silently) broke our builds, i.e.
> it was no longer setting LLVM_LIBXML2_ENABLED.
>
> Questions inline.
>
> On Tue, Oct 17, 2017 at 11:26 PM, Serguei Katkov via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> > Author: skatkov
> > Date: Tue Oct 17 23:26:39 2017
> > New Revision: 316064
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=316064&view=rev
> > Log:
> > Fix the incorrect detection of ICONV_LIBRARY_PATH
> >
> > This is introduced in rL308711.
> > Check for c library is incorrect here just because libc will be found
> always
> > and it does not mean that iconv is presented.
> >
> > Thank to Andrew Krasny for narrowing down the root cause.
>
> What this was the root cause of? I couldn't find any bug, and no
> discussion on the code review or mailing list.
>
> > Reviewers: ecbeckmann
> > Reviewed By: ecbeckmann
> > Subscribers: mgorny, llvm-commits
> > Differential Revision: https://reviews.llvm.org/D38875
> >
> > Modified:
> >     llvm/trunk/cmake/config-ix.cmake
> >
> > Modified: llvm/trunk/cmake/config-ix.cmake
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/cmake/
> config-ix.cmake?rev=316064&r1=316063&r2=316064&view=diff
> > ============================================================
> ==================
> > --- llvm/trunk/cmake/config-ix.cmake (original)
> > +++ llvm/trunk/cmake/config-ix.cmake Tue Oct 17 23:26:39 2017
> > @@ -157,7 +157,7 @@ if( NOT PURE_WINDOWS AND NOT LLVM_USE_SA
> >      set(HAVE_TERMINFO 0)
> >    endif()
> >
> > -  find_library(ICONV_LIBRARY_PATH NAMES iconv libiconv libiconv-2 c)
> > +  find_library(ICONV_LIBRARY_PATH NAMES iconv libiconv libiconv-2)
>
> On Linux, I believe libc usually provides iconv() which is why the
> check was written that way.
>
> >    set(LLVM_LIBXML2_ENABLED 0)
> >    set(LIBXML2_FOUND 0)
> >    if((LLVM_ENABLE_LIBXML2) AND ((CMAKE_SYSTEM_NAME MATCHES "Linux") AND
> (ICONV_LIBRARY_PATH) OR APPLE))
>
> But this raises lots of questions.
>
> Why do we need to check for iconv here? Isn't it enough that we find
> libxml2; i.e., if we find it, can't we assume that it's usable?
>
> And why is the iconv() check specific to Linux? Is it not needed on
> Mac, and what if the user is on FreeBSD or something else -- no
> libxml2 for them?
>
> Does anyone know what's going on here?
>
> Thanks,
> Hans
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171106/b0e32c86/attachment-0001.html>