From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E51FF469719 for ; Mon, 21 Sep 2020 23:30:00 +0300 (MSK) References: <20200921195106.96821-1-huston.mavr@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Mon, 21 Sep 2020 22:29:58 +0200 MIME-Version: 1.0 In-Reply-To: <20200921195106.96821-1-huston.mavr@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] Add missed icu symbols List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: HustonMmmavr , tarantool-patches@dev.tarantool.org, yaroslav.dynnikov@gmail.com, alexander.turenko@tarantool.org, tsafin@tarantool.org Thanks for the patch! On 21.09.2020 21:51, HustonMmmavr wrote: > After patch 03790ac55 symbols exports was changed (now we have to When we mention a commit, we use a full hash (usually), and we include the commit's title in ("..."). ... your text 03790ac5510648d1d9648bb2281857a7992d0593 ("cmake: remove dynamic-list linker option") your next text ... See 5 comments below. > export required symbols manually). Icu symbols, required by icu-date > rock (as ffi calls) are unused at linkage stage of tarantool binary > and thrown away from it so icu-date won't work (in case of tarantool > static build). This patch fixes this behaviour by adding symbols > required by icu-date rock to symbols export list. > > Close #5266 > --- > > Branch: https://github.com/tarantool/tarantool/tree/HustonMmmavr/gh-5266-icu-symbols > Issue: https://github.com/tarantool/tarantool/issues/5266 > > src/exports.c | 2 + > src/exports_icu.h | 58 ++++++++++++++ > test/box-tap/gh-5266-icu-exports.test.lua | 97 +++++++++++++++++++++++ > 3 files changed, 157 insertions(+) > create mode 100644 src/exports_icu.h > create mode 100755 test/box-tap/gh-5266-icu-exports.test.lua > > diff --git a/src/exports.c b/src/exports.c > index a3c27143e..3dfef62f0 100644 > --- a/src/exports.c > +++ b/src/exports.c > @@ -95,6 +95,7 @@ > */ > #define EXPORT(symbol) extern void symbol(void); > #include "exports.h" > +#include "exports_icu.h" > #undef EXPORT > > void ** > @@ -108,6 +109,7 @@ export_syms(void) > #define EXPORT(symbol) ((void *)symbol), > static void *symbols[] = { > #include "exports.h" > + #include "exports_icu.h" 1. Lets better not touch exports.c. Better leave it subsystem independent if possible, and not duplicate the "include". > }; > #undef EXPORT > return symbols; > diff --git a/src/exports_icu.h b/src/exports_icu.h > new file mode 100644 > index 000000000..1899a6749 > --- /dev/null > +++ b/src/exports_icu.h > @@ -0,0 +1,58 @@ > +#include > + > +/** > +* ICU libraries contains symbols with appended lib version suffix. > +* (This behaviour conifgures by --enable-renaming/disable flag when 2. conifgures -> is configured. > +* building libicu; by default --enable-renaming is used) > +* > +* To solve symbols suffix problem file was included. > +* At this file used #define macros to rename symbols. > +* For example: symobl `u_strlen` converts to `ustrlen_VER` > +* > +* Next symbols used at icu-date rock (also here added u_getVersion symbol) > +* Also we assume that icu version is >= 55, because two symbols used by > +* icu-date rock appeared at following library versions: > +* - ucal_getTimeZoneID at icu 51 > +* - udat_formatCalendar at icu 55 > +* > +* To add this symbols to export list properly (to keep backward compatibility > +* with an older versions of icu) was used define from icu library: > +* U_ICU_VERSION_MAJOR_NUM. > +* > +* Older versions of libicu, for example 4.8 (or 48) has following define: > +* #define U_ICU_VERSION_MAJOR_NUM 4 > +* From version 49 of libicu numbering changed, for example libicu 50 has > +* following define: > +* #define U_ICU_VERSION_MAJOR_NUM 50 > +* Here are links for version numbering of libicu: > +* - Changes at icu 49: http://site.icu-project.org/download/49 > +* - Version numbering: http://userguide.icu-project.org/design#TOC-Version-Numbers-in-ICU > +*/ > +EXPORT(u_strlen) > +EXPORT(u_uastrcpy) > +EXPORT(u_austrcpy) > +EXPORT(u_errorName) > +EXPORT(u_getVersion) > +EXPORT(udat_open) > +EXPORT(udat_setLenient) > +EXPORT(udat_close) > +EXPORT(udat_parseCalendar) > +#if U_ICU_VERSION_MAJOR_NUM >= 55 > + EXPORT(udat_formatCalendar) > +#endif > +EXPORT(ucal_open) > +EXPORT(ucal_close) > +EXPORT(ucal_get) > +EXPORT(ucal_set) > +EXPORT(ucal_add) > +EXPORT(ucal_clear) > +EXPORT(ucal_clearField) > +EXPORT(ucal_getMillis) > +EXPORT(ucal_setMillis) > +EXPORT(ucal_getAttribute) > +EXPORT(ucal_setAttribute) > +#if U_ICU_VERSION_MAJOR_NUM >= 51 > + EXPORT(ucal_getTimeZoneID) > +#endif > +EXPORT(ucal_setTimeZone) > +EXPORT(ucal_getNow) 3. Lets better store everything in exports.h. You can add a new section in this file separated by a comment, that these symbols are from ICU, to not scatter them among other exports. Also please sort the exports in the alphabetical order, as it is asked in exports.h comment. > diff --git a/test/box-tap/gh-5266-icu-exports.test.lua b/test/box-tap/gh-5266-icu-exports.test.lua > new file mode 100755 > index 000000000..49e012fac > --- /dev/null > +++ b/test/box-tap/gh-5266-icu-exports.test.lua 4. Since nothing depends on box here, I would move it to app-tap. > @@ -0,0 +1,97 @@ > +#!/usr/bin/env tarantool > + > +local tap = require('tap') > +local ffi = require('ffi') > + > +local test = tap.test('icu_exports') > +test:plan(2) > + > +-- Find `u_getVersion` symbol (by nm) from tarantool binary > +-- for further detection of icu version. There are two approaches: > +-- * some of icu packages appends icu version to it's symbols as suffix, > +-- so we need to simple parse it. > +-- * other packages don't append version as suffix, so in this case > +-- we have to call u_getVersion through ffi to grab icu version. > +-- On Freebsd nm requires -D option, while on Linux it's not necessary > +-- and macOS nm doesn't have this option. > +local tnt_path = arg[-1] > +local pipe = io.popen(string.format( > + 'nm %s %s | grep u_getVersion', > + jit.os == "BSD" and '-D' or '', > + tnt_path > +)) > +local u_strlen_info = pipe:read('*all') > +pipe:close() > + > +test:ok(u_strlen_info ~= '', "Finded u_getVersion symbol (to get icu version)") 5. Finded -> Found. > +local icu_version_suffix = u_strlen_info:gsub('.*u_getVersion', ''):gsub('\n', '')