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

[RDY] re-enable iconv #1370

Merged
merged 1 commit into from Nov 6, 2014

Conversation

Projects
None yet
4 participants
@aktau
Member

aktau commented Nov 1, 2014

This seems to have been disabled in the transition from vim to neovim,
re-enable it. If the disabling of iconv was intentional, why? (in which case we should delete the code that still references it).

Since this also involves the config system and I just hacked my way around it, I'd definitely like some input from @jszakmeister. How would you handle it?

@aktau

This comment has been minimized.

Member

aktau commented Nov 2, 2014

I'm still unsure whether this has anything to do with it, but I'm logging it here because I'm dealing with encodings now:

Generate a latin-9 file (I chose the euro symbol because it is representable in 1 byte in the latin-9 encoding, but not representable in latin-1 and takes 3 bytes in utf-8):

str=''
echo -n $str | iconv --to-code=LATIN-9 > euro.latin9
echo -n $str > euro.utf8

Open it with vanilla vim and nvim

# shows the euro symbol and [converted]
$ vim -c "e ++enc=latin9 euro.latin9"

# shows a question mark and [NOT converted][ILLEGAL BYTE in line 1]
$ ~/github/neovim/neovim/build/bin/nvim -c "e ++enc=latin9 euro.latin9"

So it seems that vim's and nvim's capabilities have diverged a bit.

UPDATE: The iconv patch in this PR indeed restores the functionality. Awesome.

UPDATE 2: The error message could be a bit clearer though, there was no illegal byte, just an unsupported encoding.

@jszakmeister

This comment has been minimized.

Member

jszakmeister commented Nov 4, 2014

I'm just getting back into town, but I'll try and look at this over the next couple of days. FWIW, I was curious about the omission of iconv myself when the project first started. I figured there was some method to the madness, instead of it just being an oversight. :-)

@jszakmeister

This comment has been minimized.

Member

jszakmeister commented Nov 5, 2014

Instead of keying off of APPLE, would you mind squashing this in:

diff --git a/config/CMakeLists.txt b/config/CMakeLists.txt
index 0ed430a..c488875 100644
--- a/config/CMakeLists.txt
+++ b/config/CMakeLists.txt
@@ -43,10 +43,14 @@ check_function_exists(fsync HAVE_FSYNC)
 check_function_exists(getpwent HAVE_GETPWENT)
 check_function_exists(getpwnam HAVE_GETPWNAM)
 check_function_exists(getpwuid HAVE_GETPWUID)
-if(APPLE)
+
+check_include_files(iconv.h HAVE_ICONV_H)
+check_library_exists(iconv iconv "" HAVE_ICONV_LIB)
+if(HAVE_ICONV_LIB)
   set(CMAKE_REQUIRED_LIBRARIES iconv)
 endif()
 check_function_exists(iconv HAVE_ICONV)
+
 check_function_exists(lstat HAVE_LSTAT)
 if(NOT HAVE_LSTAT)
   # os_unix.c uses lstat.c
diff --git a/src/nvim/CMakeLists.txt b/src/nvim/CMakeLists.txt
index d757bb9..f927380 100644
--- a/src/nvim/CMakeLists.txt
+++ b/src/nvim/CMakeLists.txt
@@ -156,7 +156,7 @@ else()
   endif()
 endif()

-if (APPLE)
+if(HAVE_ICONV_LIB)
   list(APPEND NVIM_LINK_LIBRARIES iconv)
 endif()

Outside of that, it looks pretty good to me.

@aktau

This comment has been minimized.

Member

aktau commented Nov 5, 2014

Instead of keying off of APPLE

Yes indeed, this is the part that bothered me most. On Apple, iconv is always present, but it has to be explicitly linked in. So that was my hackish solution.

iconv: re-enable
This seems to have been disabled in the transition from vim to neovim,
re-enable it.

@aktau aktau force-pushed the aktau:enable-iconv branch from 860aab1 to 8c5efd6 Nov 5, 2014

@aktau

This comment has been minimized.

Member

aktau commented Nov 5, 2014

@jszakmeister I've squashed in your changes, looks neat.

@jszakmeister

This comment has been minimized.

Member

jszakmeister commented Nov 6, 2014

LGTM @aktau.

@aktau

This comment has been minimized.

Member

aktau commented Nov 6, 2014

Thanks, tagging RDY. Merge at your leisure.

@aktau aktau changed the title from [RFC] re-enable iconv to [RDY] re-enable iconv Nov 6, 2014

justinmk added a commit that referenced this pull request Nov 6, 2014

@justinmk justinmk merged commit 37d608b into neovim:master Nov 6, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@justinmk

This comment has been minimized.

Member

justinmk commented Nov 6, 2014

I was hoping this would fix #1271 but seems not.

@elmart

This comment has been minimized.

Member

elmart commented Nov 7, 2014

Hi. I'm now using XCode to help me get rid of analysis warnings.
After this PR, project builds ok in the command line, but cmake generated project for XCode doesn't build, because of linking errors to iconv:

Ld xcode/bin/Debug/nvim normal x86_64
    cd /Users/eliseo/projects/os/neovim
    export MACOSX_DEPLOYMENT_TARGET=10.10
    /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk -L/Users/eliseo/projects/os/neovim/xcode/bin/Debug -F/Users/eliseo/projects/os/neovim/xcode/bin/Debug -filelist /Users/eliseo/projects/os/neovim/xcode/src/nvim/NEOVIM.build/Debug/nvim.build/Objects-normal/x86_64/nvim.LinkFileList -mmacosx-version-min=10.10 -pagezero_size 10000 -image_base 100000000 -Wl,-search_paths_first -Wl,-headerpad_max_install_names /opt/local/lib/libintl.dylib -lcurses -liconv /Users/eliseo/projects/os/neovim/.deps/usr/lib/libuv.a -ldl /Users/eliseo/projects/os/neovim/.deps/usr/lib/libmsgpack.a /Users/eliseo/projects/os/neovim/.deps/usr/lib/libluajit-5.1.a -lm -Xlinker -dependency_info -Xlinker /Users/eliseo/projects/os/neovim/xcode/src/nvim/NEOVIM.build/Debug/nvim.build/Objects-normal/x86_64/nvim_dependency_info.dat -o /Users/eliseo/projects/os/neovim/xcode/bin/Debug/nvim

Undefined symbols for architecture x86_64:
  "_libiconv", referenced from:
      _readfile in fileio.o
      _buf_write_bytes in fileio.o
      _my_iconv_open in mbyte.o
      _iconv_string in mbyte.o
  "_libiconv_close", referenced from:
      _readfile in fileio.o
      _buf_write in fileio.o
      _my_iconv_open in mbyte.o
      _convert_setup_ext in mbyte.o
  "_libiconv_open", referenced from:
      _my_iconv_open in mbyte.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Ideas?

@elmart

This comment has been minimized.

Member

elmart commented Nov 7, 2014

After make distclean, it fails on the command line too:

[ 81%] Building C object src/nvim/CMakeFiles/nvim.dir/msgpack_rpc/channel.c.o
[ 81%] Building C object src/nvim/CMakeFiles/nvim.dir/msgpack_rpc/helpers.c.o
[ 83%] Building C object src/nvim/CMakeFiles/nvim.dir/msgpack_rpc/server.c.o
Linking C executable ../../bin/nvim
Undefined symbols for architecture x86_64:
  "_libiconv", referenced from:
      _readfile in fileio.c.o
      _buf_write_bytes in fileio.c.o
      _my_iconv_open in mbyte.c.o
      _iconv_string in mbyte.c.o
  "_libiconv_close", referenced from:
      _readfile in fileio.c.o
      _buf_write in fileio.c.o
      _my_iconv_open in mbyte.c.o
      _convert_setup_ext in mbyte.c.o
  "_libiconv_open", referenced from:
      _my_iconv_open in mbyte.c.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[3]: *** [bin/nvim] Error 1
make[2]: *** [src/nvim/CMakeFiles/nvim.dir/all] Error 2
make[1]: *** [all] Error 2
make: *** [nvim] Error 2

I have libiconv 1.14 installed through macports (OS X Yosemite).
In any case, is not libiconv support supposed to be optional?

Please say if you prefer that I open a separated issue for this.

@jszakmeister

This comment has been minimized.

Member

jszakmeister commented Nov 7, 2014

I have libiconv 1.14 installed through macports (OS X Yosemite).

I wonder if that's the problem. It appears that Yosemite now comes with iconv by default, and it sounds like the one you have installed via MacPorts is not for the correct architecture.

The one that comes with Yosemite says:

:: file /usr/lib/libiconv.dylib
/usr/lib/libiconv.dylib: Mach-O universal binary with 2 architectures
/usr/lib/libiconv.dylib (for architecture x86_64):  Mach-O 64-bit dynamically linked shared library x86_64
/usr/lib/libiconv.dylib (for architecture i386):    Mach-O dynamically linked shared library i386

What does file say about your MacPorts version?

A more curious question is why did the check pass and then the actual linking fail? I suspect some other dependency is coming from MacPorts and causing the MacPorts library area to be inserted ahead of the system library path. This, in turn, is causing us to link against the wrong library.

@elmart

This comment has been minimized.

Member

elmart commented Nov 7, 2014

~/p/o/neovim git:clang-analysis-fixes-2 ❯❯❯ file /opt/local/lib/libiconv.dylib      ✭
/opt/local/lib/libiconv.dylib: Mach-O 64-bit dynamically linked shared library x86_64

Would that be a problem?

@jszakmeister

This comment has been minimized.

Member

jszakmeister commented Nov 7, 2014

Hmph. I expected it to say i386 due to this in your error message:

ld: symbol(s) not found for architecture x86_64

Can you run the full build with make VERBOSE=1 and put that in a gist somewhere? I'd like to see the command line actually being used.

@elmart

This comment has been minimized.

Member

elmart commented Nov 7, 2014

I was just doing that. Here you have it.

@jszakmeister

This comment has been minimized.

Member

jszakmeister commented Nov 7, 2014

So I believe what is happening here is that the iconv.h is being picked up out of MacPorts but you're linking against the iconv in the system area. Can you try this patch and see if it helps?

diff --git a/config/CMakeLists.txt b/config/CMakeLists.txt
index c488875..6978fc3 100644
--- a/config/CMakeLists.txt
+++ b/config/CMakeLists.txt
@@ -47,7 +47,8 @@ check_function_exists(getpwuid HAVE_GETPWUID)
 check_include_files(iconv.h HAVE_ICONV_H)
 check_library_exists(iconv iconv "" HAVE_ICONV_LIB)
 if(HAVE_ICONV_LIB)
-  set(CMAKE_REQUIRED_LIBRARIES iconv)
+  find_library(ICONV_LIBRARY NAMES iconv)
+  set(CMAKE_REQUIRED_LIBRARIES ${ICONV_LIBRARY})
 endif()
 check_function_exists(iconv HAVE_ICONV)

diff --git a/src/nvim/CMakeLists.txt b/src/nvim/CMakeLists.txt
index 5b97cf5..fac8060 100644
--- a/src/nvim/CMakeLists.txt
+++ b/src/nvim/CMakeLists.txt
@@ -158,8 +158,8 @@ else()
   endif()
 endif()

-if(HAVE_ICONV_LIB)
-  list(APPEND NVIM_LINK_LIBRARIES iconv)
+if(ICONV_LIBRARY)
+  list(APPEND NVIM_LINK_LIBRARIES ${ICONV_LIBRARY})
 endif()

 # Put these last on the link line, since multiple things may depend on them.
@aktau

This comment has been minimized.

Member

aktau commented Nov 7, 2014

I'm sure Yosemite delivers iconv, and 90% that Mavericks did too. SO http://stackoverflow.com/questions/10616724/iconv-in-mac-os-x-10-7-3-does-nothing seems to imply that it was already present in 10.7.

@elmart

This comment has been minimized.

Member

elmart commented Nov 7, 2014

@aktau It is present. The problem is not that I don't have libiconv. The problem is that I have two (system version and macports version), and somehow the build process is getting confused. Trying @jszakmeister's patch now.

@aktau

This comment has been minimized.

Member

aktau commented Nov 7, 2014

@aktau It is present. The problem is not that I don't have libiconv. The problem is that I have two (system version and macports version), and somehow the build process is getting confused. Trying @jszakmeister's patch now.

Yea :). Hope it works!

@elmart

This comment has been minimized.

Member

elmart commented Nov 7, 2014

No luck :-(
New gist at https://gist.github.com/elmart/f3b35dce2cd1427bd3e5.
Not sure if anything changed.

@jszakmeister

This comment has been minimized.

Member

jszakmeister commented Nov 7, 2014

@elmart Bummer. :-( Can you do the verbose build?

@elmart

This comment has been minimized.

Member

elmart commented Nov 7, 2014

@jszakmeister

This comment has been minimized.

Member

jszakmeister commented Nov 7, 2014

No problem. It does appear to confirm what I expected: the iconv.h is being picked up in /opt/local/include but is linking against /usr/lib/libiconv.dylib. It kind of stinks that you can't tell the compiler which iconv.h header to use. Let me think about this a bit more. CMake does have a way of saying "Check the paths first", and I think that's what we want to do here. I also think I want to move most of the iconv stuff into a FindIconv.cmake, since things are going to get more interesting. It's almost work time for me, so I'll be offline soon, but hopefully I can get to it in the morning. In the meantime, with my patch, I believe you can pass "-DICONV_LIBRARY=/opt/local/lib/libiconv.dylib" in CMAKE_EXTRA_FLAGS and it will use what you specify instead. That should hopefully fix your build for now.

@elmart

This comment has been minimized.

Member

elmart commented Nov 7, 2014

Yes! That did the trick!
Thanks very much, @jszakmeister. You saved my day.

@elmart

This comment has been minimized.

Member

elmart commented Nov 7, 2014

BTW, how do I change cmake files to do add this flag automatically? I've tried several things, but cmake still baffles me. I know I could just export env var in my command line, but I need it in cmake files for xcode to pick that.

@elmart

This comment has been minimized.

Member

elmart commented Nov 7, 2014

Ok, I just figured out. Sorry for making such noob questions, but, as I said, cmake drives me crazy. In this case, I hadn't realized CMAKE_EXTRA_FLAGS was not a CMAKE thing, but a construct of our own Makefile. When you have some time, could you advice a good tutorial on cmake? Thanks.

@jszakmeister

This comment has been minimized.

Member

jszakmeister commented Nov 7, 2014

@elmart Unfortunately, I don't know of a good tutorial, but the "Mastering CMake" book has been useful. Once you grasp some concepts, the CMake documentation becomes much more helpful.

@jszakmeister

This comment has been minimized.

Member

jszakmeister commented Nov 8, 2014

@elmart Would it be possible for you to post a copy of your build/CMakeCache.txt without my patch? I'd like to see what it getting picked up that is causing /opt/local to be added to the include path ahead of the other paths. I suspect it's libintl, but I'd like to be sure. And I'd like to see the values of some of the cached variables. Thanks!

@elmart

This comment has been minimized.

Member

elmart commented Nov 8, 2014

Here it is.

@jszakmeister

This comment has been minimized.

Member

jszakmeister commented Nov 8, 2014

Thank you.

jszakmeister added a commit to jszakmeister/neovim that referenced this pull request Nov 8, 2014

build: give priority to /sw and /opt/local on Mac OS X
Unfortunately, we can't force the specific inclusion of a header file.
So if anything add /opt/local/include to the include path--such as
libintl--then other dependencies might be drawn from /opt/local at
compile time, even though we detected them elsewhere at configure time.
This, in turn, causes issues with mixed versions, such as the iconv.h
header being pulled from /opt/local/include, but linked against the
library in /usr/lib--which can be mismatched versions.

So, despite CMake's best effort to treat /sw and /opt/local as just
another system area, we really need to give them preferential treatment.
To do this, we add them to CMAKE_PREFIX_PATH.

This fixes an issue discovered while re-enabling iconv in neovim#1370.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment