[Tarantool-patches] [PATCH v1] Block linker flag '--no-undefined'

[Tarantool-patches] [PATCH v1] Block linker flag '--no-undefined'

[Alexander V. Tikhonov]

Found that opensuse adding linker flag '--no-undefined' which produces
the fails on building tests. Decided to block this flag due to dynamic
libraries will be loaded from tarantool executable and will use symbols
from it. So it is completely okay to have unresolved symbols at build
time.

Relates to tarantool/tarantool#4562
---

Github: https://github.com/tarantool/luajit/tree/avtikhon/gh-4562-suse-block-linker-flag
Issue: https://github.com/tarantool/tarantool/issues/4562

 test/gh-4427-ffi-sandwich/CMakeLists.txt | 1 +
 test/lj-flush-on-trace/CMakeLists.txt    | 1 +
 2 files changed, 2 insertions(+)

diff --git a/test/gh-4427-ffi-sandwich/CMakeLists.txt b/test/gh-4427-ffi-sandwich/CMakeLists.txt
index 995c6bb..6028381 100644
--- a/test/gh-4427-ffi-sandwich/CMakeLists.txt
+++ b/test/gh-4427-ffi-sandwich/CMakeLists.txt
@@ -1 +1,2 @@
+string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
 build_lualib(libsandwich libsandwich.c)
diff --git a/test/lj-flush-on-trace/CMakeLists.txt b/test/lj-flush-on-trace/CMakeLists.txt
index a90452d..4f2f956 100644
--- a/test/lj-flush-on-trace/CMakeLists.txt
+++ b/test/lj-flush-on-trace/CMakeLists.txt
@@ -1 +1,2 @@
+string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
 build_lualib(libflush libflush.c)
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v1] Block linker flag '--no-undefined'

[Alexander Turenko]

LGTM except two comments below.

Please, ask Igor to do the second review.

> Block linker flag '--no-undefined'

I would highlight the fact that it is for testing libs, not the main
library / executable. Like so: '...for testing .so'.

> Found that opensuse adding linker flag '--no-undefined' which produces
> the fails on building tests. Decided to block this flag due to dynamic
> libraries will be loaded from tarantool executable and will use symbols
> from it. So it is completely okay to have unresolved symbols at build
> time.
> 
> Relates to tarantool/tarantool#4562
> ---
> 
> Github: https://github.com/tarantool/luajit/tree/avtikhon/gh-4562-suse-block-linker-flag
> Issue: https://github.com/tarantool/tarantool/issues/4562
> 
>  test/gh-4427-ffi-sandwich/CMakeLists.txt | 1 +
>  test/lj-flush-on-trace/CMakeLists.txt    | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/test/gh-4427-ffi-sandwich/CMakeLists.txt b/test/gh-4427-ffi-sandwich/CMakeLists.txt
> index 995c6bb..6028381 100644
> --- a/test/gh-4427-ffi-sandwich/CMakeLists.txt
> +++ b/test/gh-4427-ffi-sandwich/CMakeLists.txt
> @@ -1 +1,2 @@
> +string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")

Can you add the same clarification as you do for tarantool's patch?

Re: [Tarantool-patches] [PATCH v1] Block linker flag '--no-undefined'

[Igor Munkin]

Sasha,

Thanks for the patch!

On 06.07.20, Alexander V. Tikhonov wrote:
> Found that opensuse adding linker flag '--no-undefined' which produces
> the fails on building tests. Decided to block this flag due to dynamic
> libraries will be loaded from tarantool executable and will use symbols
> from it. So it is completely okay to have unresolved symbols at build
> time.

Minor: I reworded the message a bit:
| Found that OpenSUSE toolchain adds '--no-undefined' linked flag leading
| to fails while building tests. The changes suppress this flag since
| dynamic libraries are loaded via Tarantool executable and use its
| symbols. So it is completely OK to have undefined symbols at build time.
Feel free to adjust it on your own.

Also, it would be nice to add "test: " prefix to explicitly mention the
affected parts.

> 
> Relates to tarantool/tarantool#4562
> ---
> 
> Github: https://github.com/tarantool/luajit/tree/avtikhon/gh-4562-suse-block-linker-flag
> Issue: https://github.com/tarantool/tarantool/issues/4562
> 
>  test/gh-4427-ffi-sandwich/CMakeLists.txt | 1 +
>  test/lj-flush-on-trace/CMakeLists.txt    | 1 +
>  2 files changed, 2 insertions(+)
> 

Strictly saying, I see no reason to fix the problem here. These changes
are similar and one ought to add this "woodoo magic" line to every
CMakeLists.txt used for building dynamic libraries using Lua C API.
Since we are using <build_lualib> to build these extensions can you make
this change there?

> diff --git a/test/gh-4427-ffi-sandwich/CMakeLists.txt b/test/gh-4427-ffi-sandwich/CMakeLists.txt
> index 995c6bb..6028381 100644
> --- a/test/gh-4427-ffi-sandwich/CMakeLists.txt
> +++ b/test/gh-4427-ffi-sandwich/CMakeLists.txt
> @@ -1 +1,2 @@
> +string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
>  build_lualib(libsandwich libsandwich.c)
> diff --git a/test/lj-flush-on-trace/CMakeLists.txt b/test/lj-flush-on-trace/CMakeLists.txt
> index a90452d..4f2f956 100644
> --- a/test/lj-flush-on-trace/CMakeLists.txt
> +++ b/test/lj-flush-on-trace/CMakeLists.txt
> @@ -1 +1,2 @@
> +string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
>  build_lualib(libflush libflush.c)
> -- 
> 2.17.1
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v1] Block linker flag '--no-undefined'

[Alexander V. Tikhonov]

Hi Igor, thanks for the review, I've tried your suggestion and it
successfully worked. So I've updated the patch with it and commit
message as you suggested.

On Wed, Jul 08, 2020 at 02:37:26PM +0300, Igor Munkin wrote:
> Sasha,
> 
> Thanks for the patch!
> 
> On 06.07.20, Alexander V. Tikhonov wrote:
> > Found that opensuse adding linker flag '--no-undefined' which produces
> > the fails on building tests. Decided to block this flag due to dynamic
> > libraries will be loaded from tarantool executable and will use symbols
> > from it. So it is completely okay to have unresolved symbols at build
> > time.
> 
> Minor: I reworded the message a bit:
> | Found that OpenSUSE toolchain adds '--no-undefined' linked flag leading
> | to fails while building tests. The changes suppress this flag since
> | dynamic libraries are loaded via Tarantool executable and use its
> | symbols. So it is completely OK to have undefined symbols at build time.
> Feel free to adjust it on your own.
> 
> Also, it would be nice to add "test: " prefix to explicitly mention the
> affected parts.
> 

Done, thank you.

> > 
> > Relates to tarantool/tarantool#4562
> > ---
> > 
> > Github: https://github.com/tarantool/luajit/tree/avtikhon/gh-4562-suse-block-linker-flag
> > Issue: https://github.com/tarantool/tarantool/issues/4562
> > 
> >  test/gh-4427-ffi-sandwich/CMakeLists.txt | 1 +
> >  test/lj-flush-on-trace/CMakeLists.txt    | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> 
> Strictly saying, I see no reason to fix the problem here. These changes
> are similar and one ought to add this "woodoo magic" line to every
> CMakeLists.txt used for building dynamic libraries using Lua C API.
> Since we are using <build_lualib> to build these extensions can you make
> this change there?
> 

Right, checked and moved there.

> > diff --git a/test/gh-4427-ffi-sandwich/CMakeLists.txt b/test/gh-4427-ffi-sandwich/CMakeLists.txt
> > index 995c6bb..6028381 100644
> > --- a/test/gh-4427-ffi-sandwich/CMakeLists.txt
> > +++ b/test/gh-4427-ffi-sandwich/CMakeLists.txt
> > @@ -1 +1,2 @@
> > +string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
> >  build_lualib(libsandwich libsandwich.c)
> > diff --git a/test/lj-flush-on-trace/CMakeLists.txt b/test/lj-flush-on-trace/CMakeLists.txt
> > index a90452d..4f2f956 100644
> > --- a/test/lj-flush-on-trace/CMakeLists.txt
> > +++ b/test/lj-flush-on-trace/CMakeLists.txt
> > @@ -1 +1,2 @@
> > +string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
> >  build_lualib(libflush libflush.c)
> > -- 
> > 2.17.1
> > 
> 
> -- 
> Best regards,
> IM

Re: [Tarantool-patches] [PATCH v1] Block linker flag '--no-undefined'

[Alexander Turenko]

> Minor: I reworded the message a bit:
> | Found that OpenSUSE toolchain adds '--no-undefined' linked flag leading

Typo: linked -> linker.

> Strictly saying, I see no reason to fix the problem here. These changes
> are similar and one ought to add this "woodoo magic" line to every
> CMakeLists.txt used for building dynamic libraries using Lua C API.
> Since we are using <build_lualib> to build these extensions can you make
> this change there?

I missed this way. It should work, but we should care about not leaving
a test/foo subdirectory scope. I think it should be verified manually. I
would even add a comment regarding the macro behaviour: it affects
current cmake variable scope and so a user should care to don't use it
in a top level scope.

Sadly, we cannot solve the problem with properties, because flags from
the LINK_FLAGS target property are added before
CMAKE_SHARED_LINKER_FLAGS flags and because there is no --no-undefined
counterpart linker option (there is `--undefined foo` to allow certain
symbols to be undefined, but is not what we need here).

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH v1] Block linker flag '--no-undefined'

[Alexander V. Tikhonov]

Hi Alexander, thanks for the review, I've corrected the messages and
checked the usage of the functions from test/CMakeLists.txt file.

On Wed, Jul 08, 2020 at 10:22:00PM +0300, Alexander Turenko wrote:
> > Minor: I reworded the message a bit:
> > | Found that OpenSUSE toolchain adds '--no-undefined' linked flag leading
> 
> Typo: linked -> linker.
>

Corrected.

> > Strictly saying, I see no reason to fix the problem here. These changes
> > are similar and one ought to add this "woodoo magic" line to every
> > CMakeLists.txt used for building dynamic libraries using Lua C API.
> > Since we are using <build_lualib> to build these extensions can you make
> > this change there?
> 
> I missed this way. It should work, but we should care about not leaving
> a test/foo subdirectory scope. I think it should be verified manually. I
> would even add a comment regarding the macro behaviour: it affects
> current cmake variable scope and so a user should care to don't use it
> in a top level scope.
>

I've added the the message below to the test/CMakeLists.txt file just
before the change:

 | # WARNING: This change affects current cmake variable scope and so
 | #          a user should care to don't use it in a top level scope.

Also I've checked all found fuctions from test/CMakeLists.txt file to be
sure that its in use only in test/* scopes:

$ for f in `grep "^function(" test/CMakeLists.txt | sed "s#^function(##g" | awk '{print($1)}'` ; do echo "========== Checking $f" ; grep -RI $f 2>/dev/null ; done
========== Checking build_module
cmake/module.cmake:function(rebuild_module_api)
src/CMakeLists.txt:rebuild_module_api(${api_headers})
test/app/CMakeLists.txt:build_module(loaderslib loaderslib.c)
test/CMakeLists.txt:function(build_module module files)
test/box/CMakeLists.txt:build_module(function1 function1.c)
test/box/CMakeLists.txt:build_module(reload1 reload1.c)
test/box/CMakeLists.txt:build_module(reload2 reload2.c)
test/box/CMakeLists.txt:build_module(tuple_bench tuple_bench.c)
test/app-tap/CMakeLists.txt:build_module(module_api module_api.c)
========== Checking build_lualib
test/CMakeLists.txt:function(build_lualib lib sources)
test/luajit-tap/lj-flush-on-trace/CMakeLists.txt:build_lualib(libflush libflush.c)
test/luajit-tap/gh-4427-ffi-sandwich/CMakeLists.txt:build_lualib(libsandwich libsandwich.c)
third_party/luajit/test/lj-flush-on-trace/CMakeLists.txt:build_lualib(libflush libflush.c)
third_party/luajit/test/gh-4427-ffi-sandwich/CMakeLists.txt:build_lualib(libsandwich libsandwich.c)

As seen above all usages of "Checking <foo>"are inside the test/ scope
except "rebuild_module_api" which is not from the checking file, but
from the src/CMakeLists.txt file and it not interesting for us.

> Sadly, we cannot solve the problem with properties, because flags from
> the LINK_FLAGS target property are added before
> CMAKE_SHARED_LINKER_FLAGS flags and because there is no --no-undefined
> counterpart linker option (there is `--undefined foo` to allow certain
> symbols to be undefined, but is not what we need here).
> 

Agree.

> WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH v1] Block linker flag '--no-undefined'

[Igor Munkin]

Sasha,

Thanks for the patch! I added the patch from upstream to point several
nits.

================================================================================

    test: fix for OpenSuSE tests build

    Found that OpenSUSE toolchain adds '--no-undefined' linker flag leading

Typo: The correct writing of the distro is openSUSE[1], so please fix it
both in commit subject and commit message.
| s/OpenS[uU]SE/openSUSE/g.

    to fails while building tests. The changes suppress this flag since
    dynamic libraries are loaded via Tarantool executable and use its
    symbols. So it is completely OK to have undefined symbols at build time.
    Feel free to adjust it on your own.

Typo: The last line was for you, not for the message, please drop it.


    Needed for #4562

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 697d1b21d..c3cd93066 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -23,6 +23,14 @@ endfunction()
 add_compile_flags("C;CXX"
     "-Wno-unused-parameter")
 
+# WARNING: This change affects current cmake variable scope and so
+#          a user should care to don't use it in a top level scope.
+# The dynamic libraries will be loaded from tarantool executable
+# and will use symbols from it. So it is completely okay to have
+# unresolved symbols at build time.

Minor: you can use the same part from the commit message, but I'm not
insisting. It just looks rational to me.

+string(REPLACE "-Wl,--no-undefined" ""
+	CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")

Please adjust the indentation considering the code nearby: use 4 spaces
instead of tab.

+
 if(POLICY CMP0037)
     if(CMAKE_VERSION VERSION_LESS 3.11)
         # cmake below 3.11 reserves name test. Use old policy.

================================================================================

Otherwise, LGTM.

[1]: https://www.opensuse.org/

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v1] Block linker flag '--no-undefined'

[Kirill Yukhin]

Hello,

On 06 июл 15:51, Alexander V. Tikhonov wrote:
> Found that opensuse adding linker flag '--no-undefined' which produces
> the fails on building tests. Decided to block this flag due to dynamic
> libraries will be loaded from tarantool executable and will use symbols
> from it. So it is completely okay to have unresolved symbols at build
> time.
> 
> Relates to tarantool/tarantool#4562
> ---
> 
> Github: https://github.com/tarantool/luajit/tree/avtikhon/gh-4562-suse-block-linker-flag
> Issue: https://github.com/tarantool/tarantool/issues/4562

I've checked your patch into 1.10, 2.3, 2.4 and master.

--
Regards, Kirill Yukhin