Skip to content
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

Cleanup iconv build system flags #3457

Closed
equalsraf opened this issue Oct 14, 2015 · 8 comments
Closed

Cleanup iconv build system flags #3457

equalsraf opened this issue Oct 14, 2015 · 8 comments
Labels
platform:windows refactor changes that are not features or bugfixes
Milestone

Comments

@equalsraf
Copy link
Contributor

equalsraf commented Oct 14, 2015

iconv support was reenabled in #1370 but the iconv detection and code could use some cleanup. A quick grep through the code shows several iconv related flags

  1. USE_ICONV is defined in src/nvim/iconv.h and can be true if we are linked against iconv or if we support dynamic loading of iconv
  2. HAVE_ICONV_H is defined by config/CMakeLists.txt if the header is found
  3. likewise HAVE_ICONV is defined config/CMakeLists.txt, but it is based on the cmake check in CMakeLists.txt (see Iconv_FOUND)
  4. DYNAMIC_ICONV is meant for loading the iconv dll at runtime (even if the header is not available at build time), but it is never set in Neovim - which makes sense for now since this one is only meant for Windows (mbyte.c). [RFC] remove DYNAMIC_ICONV #10708

Do we want to continue supporting DYNAMIC_ICONV? Vim uses dynamic loading and distributes the iconv dll separately - If not, the code can be removed, if we want to keep it we need to check some Vim changes (vim/vim/issues/440).

At least some of this can be simplified in CMakeLists.txt and config/CMakeLists.txt if the FindIconv.cmake is changed to handle the header flag. @justinmk this last bit can be tagged as entry level.

@justinmk justinmk added entry-level refactor changes that are not features or bugfixes labels Oct 14, 2015
@laserswald

This comment has been minimized.

@equalsraf

This comment has been minimized.

@laserswald

This comment has been minimized.

@ghost

This comment has been minimized.

@equalsraf

This comment has been minimized.

@justinmk
Copy link
Member

We should distribute iconv with the Windows build.

@brcolow
Copy link
Contributor

brcolow commented Dec 23, 2015

This looks interesting:
https://github.com/mlocati/gettext-iconv-windows

@justinmk justinmk added this to the 0.2 milestone Jan 17, 2017
@justinmk justinmk modified the milestones: 0.3, 0.2 Feb 4, 2017
@rafin

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform:windows refactor changes that are not features or bugfixes
Projects
None yet
Development

No branches or pull requests

5 participants