From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-f193.google.com (mail-oi1-f193.google.com [209.85.167.193]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6556E469719 for ; Mon, 28 Sep 2020 01:39:55 +0300 (MSK) Received: by mail-oi1-f193.google.com with SMTP id x69so9529164oia.8 for ; Sun, 27 Sep 2020 15:39:55 -0700 (PDT) MIME-Version: 1.0 References: <20200923110251.33201-1-huston.mavr@gmail.com> In-Reply-To: From: Alexandr Barulev Date: Mon, 28 Sep 2020 01:39:41 +0300 Message-ID: Content-Type: multipart/alternative; boundary="00000000000075363605b0533ad4" Subject: Re: [Tarantool-patches] [PATCH v2] Add missed icu symbols List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: Alexander Turenko , tarantool-patches@dev.tarantool.org, Yaroslav Dynnikov --00000000000075363605b0533ad4 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hello, thanks for the review of new patch version! I've fixed commit message (added `build` prefix) and rebased from master. I think commit, mentionied at patch message ("cmake: remove dynamic-list linker option") is related to #5266 issue. It's because symbols exports from required libs into tarantool binary earlier were configured with two options: by `--whole-archive` option; and `--dynamic-list,${exports_file}` option, where exports_file was created with use of mkexports script (symbols was grabbed by nm -D from shared libraries). Also, I've checkouted "cmake: remove dynamic-list linker option" commit, built static tarantool and ran icu-date tests. In result tests failed with `undefined symbol` errors. After that, I checkouted previous commit, built tarantool again and icu-date tests succeed =D0=BF=D1=82, 25 =D1=81=D0=B5=D0=BD=D1=82. 2020 =D0=B3. =D0=B2 00:10, Vladi= slav Shpilevoy : > Hi! Thanks for the patch! > > I would add 'build: ' prefix to the commit message. > > On 23.09.2020 13:02, HustonMmmavr wrote: > > After patch 03790ac5510648d1d9648bb2281857a7992d0593 ("cmake: remove > > dynamic-list linker option") symbols exports was changed (now we have > > to export required symbols manually). > > Actually after some thinking I am not sure it is because of that commit. > Symbols exports were always done manually. That commit changed which > symbols were hidden. So essentially it extended the exported symbols > set, not shrunk it. > > And that makes me wonder how could it lead to #5266? > > > 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). > > They would have been thrown anyway. It is not related to the dynamic-list > option removed in the mentioned commit. > > > This patch fixes this behaviour by adding symbols > > required by icu-date rock to symbols export list. > > > > Close #5266 > --00000000000075363605b0533ad4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello, thanks for the review of new patch version!

= I've fixed commit message (added `build` prefix) and rebased
from ma= ster.

I think commit, mentionied at patch message ("cmake: remo= ve
dynamic-list linker option") is related to #5266 issue.
It= 9;s because symbols exports from required libs into tarantool binary
ear= lier were configured with two options:
by `--whole-archive` option;
a= nd `--dynamic-list,${exports_file}` option, where exports_file was created<= br>with use of mkexports script (symbols was grabbed by nm -D from sharedlibraries).

Also, I've checkouted "cmake: remove dynamic-= list linker option"
commit, built static tarantool and ran icu-date= tests. In result
tests failed with `undefined symbol` errors.
After = that, I checkouted previous commit, built tarantool again
and icu-date t= ests succeed

=D0=BF=D1=82, 25 =D1=81=D0=B5=D0=BD=D1=82. 2020 =D0=B3. =D0=B2 = 00:10, Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
Hi! Thanks for the patch!

I would add 'build: ' prefix to the commit message.

On 23.09.2020 13:02, HustonMmmavr wrote:
> After patch 03790ac5510648d1d9648bb2281857a7992d0593 ("cmake: rem= ove
> dynamic-list linker option") symbols exports was changed (now we = have
> to export required symbols manually).

Actually after some thinking I am not sure it is because of that commit. Symbols exports were always done manually. That commit changed which
symbols were hidden. So essentially it extended the exported symbols
set, not shrunk it.

And that makes me wonder how could it lead to #5266?

> 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 taranto= ol
> static build).

They would have been thrown anyway. It is not related to the dynamic-list option removed in the mentioned commit.

> This patch fixes this behaviour by adding symbols
> required by icu-date rock to symbols export list.
>
> Close #5266
--00000000000075363605b0533ad4--