-
-
Notifications
You must be signed in to change notification settings - Fork 86
meson: set -liconv implicitly when iconv found #1186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand any of this.
Meson's builtin
dependency('iconv')
attempts to search two different providers of iconv:#include <iconv.h>
existsThe former case cannot and will not successfully compile and link if -liconv exists, since if -liconv exists then iconv.h will
#define iconv_open libiconv_open
and linking will fail unless you providelibiconv_open
. And libc only providesiconv_open
, but-liconv
provideslibiconv_open
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this block already occurs inside
if iconv.found()
, it stands to reason that if-liconv
exists, the iconv dependency already contains it. But, you could double check:It will print either "builtin" or "system", and in the latter case it comes with
-liconv
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where precisely does one get
-liconv
from? It is only relevant to systems NOT using GNU glibc. Arch Linux does not support non-glibc installation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue is at
fvwm3/meson.build
Line 129 in 4592849
libunistring
is installed, that check will fire and setHAVE_LIBCHARSET
, which will lead tolocale_charset
being used, but nothing specifies we should link against libunistring.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @thesamesam -- thanks for your comment here.
I looked into this, and outside of ArchLinux, also installed
libunistring
. The issue did not occur, but still seems present on ArchLinux.So my concern is what's best to fix this. Forgive me -- either your original premise here is incorrect -- or, it's ArchLinux speciic somehow.
Either way, we clearly need a solution which solves both of these.
I'll happily take any suggestions. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look and try reproduce in a container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To obtain an undefined reference to
locale_charset
as the user had, we have to refer to it in fvwm3.The only place we do that is in
libs/FlocaleCharset.c
with a call toFlocale_charset()
with:If I run
pacman -S libiconv
(andlibunistring
is already installed in this container, it seems), then the build fails for me with:I think what makes it Arch-specific here is that it's unusual to allow
libiconv
to be installed on a system with glibc installed, because it serves no purpose (and may well confuse configure checks like this). Iflibiconv
isn't installed, then the check forlibcharset.h
is never going to succeed.What confused me is that libunistring also provides
locale_charset
if you want it.So, all that said: if
libcharset.h
is installed (from libiconv),HAVE_LIBCHARSET
gets set, and then we blow up when linking because we don't add-liconv
because Meson already checked if you need anything for the usual iconv functions (following Eli's algorithm at #1186 (comment)) and found it only needs libc.What's not clear to me is what the cleanest way of fixing this is yet.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two different headers, and three libraries, that provide a function called "locale_charset":
#include <unistring/localcharset.h>
&&gcc -lunistring
#include <localcharset.h>
&&gcc -lcharset
#include <localcharset.h>
&&gcc -liconv
Case 1 is provided by the libunistring software. Cases 3 and 3 are both provided by the libiconv software
I can't understand why case number 3 is even valid. I suspect @bhaible accidentally broke libiconv at some point and then had to preserve this forever. The ChangeLog inscrutably mentions GNU bash without any details.
The reason it doesn't matter whether you install libunistring is because you are not looking for the header name from libunistring!!! Therefore if libunistring is installed you don't detect locale_charset at all... only if libiconv is installed. If you do want libiconv's copy of locale_charset then maybe at least you should look for the correct library name, which is NOT
-liconv
but rather-lcharset
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
locale_charset
function exists becausenl_langinfo (CODESET)
, cf. https://www.gnu.org/software/gnulib/manual/html_node/nl_005flanginfo.htmllocale_charset
which maps system-dependent encoding names to GNU canonical encoding names (e.g. "ISO8859-1" → "ISO-8859-1", "UTF8" → "UTF-8").locale_charset
is part of the GNU libunistring API, https://www.gnu.org/software/libunistring/manual/html_node/uniconv_002eh.html, but nothing in fvwm3 references libunistring, therefore there is no need to talk about it here.The only other library that installs a header file that declares
locale_charset
is GNU libiconv. It does so in two header files,libcharset.h
andlocalcharset.h
. fvwm3 includeslibcharset.h
before usinglocale_charset ()
, which is fine.GNU libiconv, however, is never supposed to be installed on GNU systems (such as Arch Linux or Gentoo with glibc). Therefore, on GNU systems, instead of asking "where can we get
locale_charset
from", the question should be "why is it attempting to uselocale_charset
? it should be usingnl_langinfo (CODESET)
instead".In other words, fvwm3 should use
locale_charset
if and only if GNU libiconv has been found and is usable (i.e. you can link with it).Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fvwm3/libs/FlocaleCharset.c
Lines 605 to 616 in 6370adf
fvwm3/meson.build
Line 147 in 6370adf
fvwm3/libs/FlocaleCharset.h
Lines 14 to 24 in 6370adf
It already is kinda doing that, but confusingly tries using header presence as a sentinel for "not GNU libc" and confuses it with implicit -liconv.