[tarantool-patches] [PATCH] add FindICONV and iconv wrapper

[tarantool-patches] [PATCH] add FindICONV and iconv wrapper

[Konstantin Belyavskiy]

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.

Closes #3441
---
ticket: https://github.com/tarantool/tarantool/issues/3441
branch: https://github.com/tarantool/tarantool/tree/kbelyavs/gh-3441-fix-linking-with-iconv

 CMakeLists.txt        |  6 ++++++
 cmake/FindICONV.cmake | 43 ++++++++++++++++++++++++++++++++++++++++
 extra/exports         |  3 +++
 src/CMakeLists.txt    |  9 +++------
 src/lua/iconv.lua     | 41 +++++++++-----------------------------
 src/lua/iconv_wrap.c  | 23 ++++++++++++++++++++++
 src/lua/iconv_wrap.h  | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 141 insertions(+), 38 deletions(-)
 create mode 100644 cmake/FindICONV.cmake
 create mode 100644 src/lua/iconv_wrap.c
 create mode 100644 src/lua/iconv_wrap.h

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..fd90b0cd2
--- /dev/null
+++ b/cmake/FindICONV.cmake
@@ -0,0 +1,43 @@
+# - Find the readline include files and libraries
+# - Include finding of termcap or curses
+#
+# ICONV_FOUND
+# ICONV_INCLUDE_DIR
+# ICONV_LIBRARIES
+#
+
+find_library(ICONV_LIBRARY iconv)
+find_path(ICONV_INCLUDE_DIR iconv.h)
+if(NOT ICONV_LIBRARY)
+    message(FATAL_ERROR "iconv library not found")
+endif()
+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")
+    else()
+        message(WARNING "ICONV does not run")
+    endif()
+endif()
+
+mark_as_advanced(ICONV_INCLUDE_DIRS ICONV_LIBRARIES)
diff --git a/extra/exports b/extra/exports
index 61a2e0a54..4b348f5e0 100644
--- a/extra/exports
+++ b/extra/exports
@@ -38,6 +38,9 @@ title_set_custom
 title_get_custom
 title_set_status
 title_get_status
+iconv_wrap_open
+iconv_wrap_close
+iconv_wrap
 exception_get_string
 exception_get_int
 
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 8ab09e968..ed66f1101 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/iconv_wrap.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..426fbbefd 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 iconv_wrap_open(const char *tocode, const char *fromcode);
+void    iconv_wrap_close(iconv_t cd);
+size_t  iconv_wrap(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.iconv_wrap(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.iconv_wrap(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.iconv_wrap(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.iconv_wrap_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.iconv_wrap_open(to, from)
     if iconv == conv_rv_error then
         error('iconv: '..errno.strerror())
     end
diff --git a/src/lua/iconv_wrap.c b/src/lua/iconv_wrap.c
new file mode 100644
index 000000000..2cf6be883
--- /dev/null
+++ b/src/lua/iconv_wrap.c
@@ -0,0 +1,23 @@
+#include <iconv.h>
+#include "iconv_wrap.h"
+
+iconv_t
+iconv_wrap_open(const char *tocode, const char *fromcode)
+{
+        return iconv_open(tocode, fromcode);
+}
+
+int
+iconv_wrap_close(iconv_t cd)
+{
+        return iconv_close(cd);
+}
+
+size_t
+iconv_wrap(iconv_t cd,
+           char **inbuf, size_t *inbytesleft,
+           char **outbuf, size_t *outbytesleft)
+{
+        return iconv(cd, inbuf, inbytesleft,
+                     outbuf, outbytesleft);
+}
diff --git a/src/lua/iconv_wrap.h b/src/lua/iconv_wrap.h
new file mode 100644
index 000000000..03aee40e4
--- /dev/null
+++ b/src/lua/iconv_wrap.h
@@ -0,0 +1,54 @@
+#ifndef TARANTOOL_LUA_ICONV_H_INCLUDED
+#define TARANTOOL_LUA_ICONV_H_INCLUDED
+/*
+ * Copyright 2010-2015, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+struct iconv_t;
+
+iconv_t
+iconv_wrap_open(const char *tocode, const char *fromcode);
+
+int
+iconv_wrap_close(iconv_t cd);
+
+size_t
+iconv_wrap(iconv_t cd,
+           char **inbuf, size_t *inbytesleft,
+           char **outbuf, size_t *outbytesleft);
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
+#endif /* TARANTOOL_LUA_ICONV_H_INCLUDED */
-- 
2.14.3 (Apple Git-98)

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

[Georgy Kirichenko]

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

Mostly good, but see on my comments bellow

On Friday, July 20, 2018 7:50:52 PM MSK Konstantin Belyavskiy wrote:
> Fixing build under FreeBSD:
> +# - Find the readline include files and libraries
> +# - Include finding of termcap or curses
Wrong comment

> +iconv_wrap_open
> +iconv_wrap_close
> +iconv_wrap
I would be like if you renamed this using pattern tnt_<name>
>  exception_get_string
>  exception_get_int
> 
>       lua/errno.c
> +     lua/iconv_wrap.c
I would be like if you used name like tnt_iconv.c
>       lua/socket.c
>       lua/pickle.c
>       lua/fio.c

> diff --git a/src/lua/iconv_wrap.h b/src/lua/iconv_wrap.h
> new file mode 100644
> index 000000000..03aee40e4
> --- /dev/null
> +++ b/src/lua/iconv_wrap.h
I am do not really sure we need this file

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

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

[Konstantin Belyavskiy]

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

Georgy, thank you for the review.
Please take a look at updated patch. List of changes:
- update wrong comment
- rename file and functions iconv_wrap* to tnt_iconv*
- remove unnecessary include file

>Понедельник, 23 июля 2018, 16:17 +03:00 от Georgy Kirichenko <georgy@tarantool.org>:
>
>Mostly good, but see on my comments bellow
>
>On Friday, July 20, 2018 7:50:52 PM MSK Konstantin Belyavskiy wrote:
>> Fixing build under FreeBSD:
>> +# - Find the readline include files and libraries
>> +# - Include finding of termcap or curses
>Wrong comment
>
>> +iconv_wrap_open
>> +iconv_wrap_close
>> +iconv_wrap
>I would be like if you renamed this using pattern tnt_<name>
>>  exception_get_string
>>  exception_get_int
>> 
>>       lua/errno.c
>> +     lua/iconv_wrap.c
>I would be like if you used name like tnt_iconv.c
>>       lua/socket.c
>>       lua/pickle.c
>>       lua/fio.c
>
>> diff --git a/src/lua/iconv_wrap.h b/src/lua/iconv_wrap.h
>> new file mode 100644
>> index 000000000..03aee40e4
>> --- /dev/null
>> +++ b/src/lua/iconv_wrap.h
>I am do not really sure we need this file


Best regards,
Konstantin Belyavskiy
k.belyavskiy@tarantool.org

[-- Attachment #2: Type: text/html, Size: 1842 bytes --]

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

[Kirill Yukhin]

Hello,
On 24 июл 17:39, Konstantin Belyavskiy wrote:
> Georgy, thank you for the review.
> Please take a look at updated patch. List of changes:
> - update wrong comment
> - rename file and functions iconv_wrap* to tnt_iconv*
> - remove unnecessary include file
Could you pls attach an updated patch in future mails?

--
Regards, Kirill Yukhin