[tarantool-patches] Re: [PATCH v2] add FindICONV and iconv wrapper

[Georgy Kirichenko <georgy@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 8732 bytes --]

LGTM, thanks

On Wednesday, August 1, 2018 6:49:31 PM MSK Konstantin Belyavskiy wrote:
> Fixing build under FreeBSD: Undefined symbol "iconv_open"
> Add compile time build check with FindICONV.cmake
> and a wrapper to import relevant symbol names with include file.
> 
> Changes in V2:
> - update wrong comment
> - rename file and functions iconv_wrap* to tnt_iconv*
> - remove unnecessary include file
> 
> Resend branch from 23/07 since it was a reply with request to
> attach a patch to a mail and then further discussion stops.
> 
> Closes #3441
> ---
> ticket: https://github.com/tarantool/tarantool/issues/3441
> branch:
> https://github.com/tarantool/tarantool/tree/kbelyavs/gh-3441-fix-linking-wi
> th-iconv
> 
>  CMakeLists.txt        |  6 ++++++
>  cmake/FindICONV.cmake | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  extra/exports         |  3 +++
>  src/CMakeLists.txt    |  9 +++------
>  src/lua/iconv.lua     | 41 +++++++++--------------------------------
>  src/lua/tnt_iconv.c   | 21 +++++++++++++++++++++
>  6 files changed, 86 insertions(+), 38 deletions(-)
>  create mode 100644 cmake/FindICONV.cmake
>  create mode 100644 src/lua/tnt_iconv.c
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index d0cae0f01..cea4a7d3e 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -299,6 +299,12 @@ find_package(CURL)
>  set(Readline_FIND_REQUIRED ON)
>  find_package(Readline)
> 
> +#
> +# ICONV
> +#
> +set(ICONV_FIND_REQUIRED ON)
> +find_package(ICONV)
> +
>  #
>  # ICU
>  #
> diff --git a/cmake/FindICONV.cmake b/cmake/FindICONV.cmake
> new file mode 100644
> index 000000000..2ae9c8197
> --- /dev/null
> +++ b/cmake/FindICONV.cmake
> @@ -0,0 +1,44 @@
> +# - Find the iconv include file and optional library
> +#
> +# ICONV_INCLUDE_DIRS
> +# ICONV_LIBRARIES
> +#
> +
> +if (TARGET_OS_LINUX)
> +    set(ICONV_LIBRARY "")
> +else()
> +    find_library(ICONV_LIBRARY iconv)
> +    if(NOT ICONV_LIBRARY)
> +        message(WARNING "iconv library not found")
> +        set(ICONV_LIBRARY "")
> +    endif()
> +endif()
> +find_path(ICONV_INCLUDE_DIR iconv.h)
> +if(NOT ICONV_INCLUDE_DIR)
> +    message(FATAL_ERROR "iconv include header not found")
> +endif()
> +set(ICONV_LIBRARIES ${ICONV_LIBRARY})
> +set(ICONV_INCLUDE_DIRS ${ICONV_INCLUDE_DIR})
> +set(CMAKE_REQUIRED_LIBRARIES ${ICONV_LIBRARIES})
> +set(CMAKE_REQUIRED_INCLUDES ${ICONV_INCLUDE_DIRS})
> +check_c_source_runs("
> +    #include <iconv.h>
> +    int main()
> +    {
> +        void* foo = iconv_open(\"UTF-8\", \"ISO-8859-1\");
> +        iconv_close(foo);
> +        return 0;
> +    }
> +    " ICONV_RUNS)
> +set(CMAKE_REQUIRED_LIBRARIES "")
> +set(CMAKE_REQUIRED_INCLUDES "")
> +if (NOT DEFINED ICONV_RUNS_EXITCODE OR ICONV_RUNS_EXITCODE)
> +    unset(ICONV_LIBRARIES)
> +    unset(ICONV_INCLUDE_DIRS)
> +    set(ICONV_FOUND false)
> +    if (ICONV_FIND_REQUIRED)
> +        message(FATAL_ERROR "ICONV does not run")
> +    endif()
> +endif()
> +
> +mark_as_advanced(ICONV_INCLUDE_DIRS ICONV_LIBRARIES)
> diff --git a/extra/exports b/extra/exports
> index 61a2e0a54..490118e36 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -38,6 +38,9 @@ title_set_custom
>  title_get_custom
>  title_set_status
>  title_get_status
> +tnt_iconv_open
> +tnt_iconv_close
> +tnt_iconv
>  exception_get_string
>  exception_get_int
> 
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 8ab09e968..b1c4cd711 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> @@ -12,6 +12,7 @@ include_directories(${LIBYAML_INCLUDE_DIRS})
>  include_directories(${MSGPUCK_INCLUDE_DIRS})
>  include_directories(${CURL_INCLUDE_DIRS})
>  include_directories(${ICU_INCLUDE_DIRS})
> +include_directories(${ICONV_INCLUDE_DIRS})
> 
>  set(LIBUTIL_FREEBSD_SRC ${CMAKE_SOURCE_DIR}/third_party/libutil_freebsd)
>  include_directories(${LIBUTIL_FREEBSD_SRC})
> @@ -159,6 +160,7 @@ set (server_sources
>       lua/msgpack.c
>       lua/utils.c
>       lua/errno.c
> +     lua/tnt_iconv.c
>       lua/socket.c
>       lua/pickle.c
>       lua/fio.c
> @@ -221,6 +223,7 @@ set (common_libraries
>      ${READLINE_LIBRARIES}
>      ${OPENSSL_LIBRARIES}
>      ${CURL_LIBRARIES}
> +    ${ICONV_LIBRARIES}
>  )
> 
>  if (TARGET_OS_LINUX OR TARGET_OS_DEBIAN_FREEBSD)
> @@ -234,12 +237,6 @@ if (TARGET_OS_FREEBSD AND NOT TARGET_OS_DEBIAN_FREEBSD)
> else()
>          set (common_libraries ${common_libraries} ${INTL})
>      endif()
> -    find_library (ICONV iconv)
> -    if (NOT ICONV)
> -        message(FATAL_ERROR "iconv library not found")
> -    else()
> -        set (common_libraries ${common_libraries} ${ICONV})
> -    endif()
>  endif()
> 
>  set (common_libraries ${common_libraries} ${LIBUUID_LIBRARIES})
> diff --git a/src/lua/iconv.lua b/src/lua/iconv.lua
> index 6a6bfeb76..ade66d760 100644
> --- a/src/lua/iconv.lua
> +++ b/src/lua/iconv.lua
> @@ -4,17 +4,10 @@ local buffer = require('buffer')
> 
>  ffi.cdef[[
>  typedef struct iconv *iconv_t;
> -iconv_t iconv_open(const char *tocode, const char *fromcode);
> -void    iconv_close(iconv_t cd);
> -size_t  iconv(iconv_t cd, const char **inbuf, size_t *inbytesleft,
> -              char **outbuf, size_t *outbytesleft);
> -/*
> - * add prefix 'lib' under FreeBSD
> - */
> -iconv_t libiconv_open(const char *tocode, const char *fromcode);
> -void    libiconv_close(iconv_t cd);
> -size_t  libiconv(iconv_t cd, const char **inbuf, size_t *inbytesleft,
> -                 char **outbuf, size_t *outbytesleft);
> +iconv_t tnt_iconv_open(const char *tocode, const char *fromcode);
> +void    tnt_iconv_close(iconv_t cd);
> +size_t  tnt_iconv(iconv_t cd, const char **inbuf, size_t *inbytesleft,
> +                   char **outbuf, size_t *outbytesleft);
>  ]]
> 
>  local iconv_t         = ffi.typeof('struct iconv')
> @@ -23,22 +16,6 @@ local cchar_ptr_arr_t = ffi.typeof('const char *[1]')
>  local cchar_ptr_t     = ffi.typeof('const char *')
>  local size_t_arr_t    = ffi.typeof('size_t [1]')
> 
> -local _iconv_open
> -local _iconv_close
> -local _iconv
> -
> --- To fix #3073, BSD iconv implementation is not fully
> --- compatible with iconv, so use external iconv.so lib
> -if jit.os == 'BSD' then
> -    _iconv_open = ffi.C.libiconv_open
> -    _iconv_close = ffi.C.libiconv_close
> -    _iconv = ffi.C.libiconv
> -else
> -    _iconv_open = ffi.C.iconv_open
> -    _iconv_close = ffi.C.iconv_close
> -    _iconv = ffi.C.iconv
> -end
> -
>  local E2BIG    = errno['E2BIG']
>  local EINVAL   = errno['EINVAL']
>  local EILSEQ   = errno['EILSEQ']
> @@ -64,10 +41,10 @@ local function iconv_convert(iconv, data)
>      while data_left[0] > 0 do
>          buf_ptr[0]  = buf:reserve(output_len)
>          buf_left[0] = buf:unused()
> -        local res = _iconv(iconv, data_ptr, data_left,
> +        local res = ffi.C.tnt_iconv(iconv, data_ptr, data_left,
>                                  buf_ptr, buf_left)
>          if res == ffi.cast('size_t', -1) and errno() ~= E2BIG then
> -            _iconv(iconv, nil, nil, nil, nil)
> +            ffi.C.tnt_iconv(iconv, nil, nil, nil, nil)
>              if errno() == EINVAL then
>                  error('Invalid multibyte sequence')
>              end
> @@ -80,7 +57,7 @@ local function iconv_convert(iconv, data)
>      end
> 
>      -- iconv function sets cd's conversion state to the initial state
> -    _iconv(iconv, nil, nil, nil, nil)
> +    ffi.C.tnt_iconv(iconv, nil, nil, nil, nil)
>      local result = ffi.string(buf.rpos, buf:size())
>      buf:reset()
>      return result
> @@ -88,7 +65,7 @@ end
> 
>  local iconv_mt = {
>      __call = iconv_convert,
> -    __gc = _iconv_close,
> +    __gc = ffi.C.tnt_iconv_close,
>      __tostring = function(iconv) return string.format("iconv: %p", iconv)
> end }
> 
> @@ -98,7 +75,7 @@ local function iconv_new(to, from)
>      if type(to) ~= 'string' or type(from) ~= 'string' then
>          error('Usage: iconv.new("CP1251", "KOI8-R")')
>      end
> -    local iconv = _iconv_open(to, from)
> +    local iconv = ffi.C.tnt_iconv_open(to, from)
>      if iconv == conv_rv_error then
>          error('iconv: '..errno.strerror())
>      end
> diff --git a/src/lua/tnt_iconv.c b/src/lua/tnt_iconv.c
> new file mode 100644
> index 000000000..c91fef140
> --- /dev/null
> +++ b/src/lua/tnt_iconv.c
> @@ -0,0 +1,21 @@
> +#include <iconv.h>
> +
> +iconv_t
> +tnt_iconv_open(const char *tocode, const char *fromcode)
> +{
> +        return iconv_open(tocode, fromcode);
> +}
> +
> +int
> +tnt_iconv_close(iconv_t cd)
> +{
> +        return iconv_close(cd);
> +}
> +
> +size_t
> +tnt_iconv(iconv_t cd, char **inbuf, size_t *inbytesleft,
> +          char **outbuf, size_t *outbytesleft)
> +{
> +        return iconv(cd, inbuf, inbytesleft,
> +                     outbuf, outbytesleft);
> +}


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]