[tarantool-patches] Re: [PATCH v3] Tarantool static build ability

[Georgy Kirichenko <georgy@tarantool.org>]

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

On Wednesday, August 29, 2018 4:25:47 AM MSK Alexander Turenko wrote:
I sent fixed branch and a message.
Please look on my comments inline
> Hi, Georgy!
> 
> I have minor notes on the code and some experimenting results. Please,
> especially consider attempt to use docker at end of the email.
> 
> I don't think I can add more value on the next review rounds, so,
> please, proceed with the next reviewer (I guess Kirill Yu.) if the
> process allows it.
> 
> WBR, Alexander Turenko.
> 
> ---- Updates ----
> 
> Included cmake file has the same line at the line 374 (fixed in the our
> repo, of cource, see the first review, where I show the diff).
Fixed

> Are not pthread should follows gomp? nm reports undefined static symbols
> starts with 'pthread_' in libgomp.a.
Fixed

> libcurl.a can be linked with other libraries (nghttp2 at least) and we
> possibly should take care of it to perform static build successfully on
Fixed

> CURSES_CURSES_LIBRARY is not used.
> CURSES_NCURSES_LIBRARY is not used.
> CURSES_FORM_LIBRARY is not used.
> 
> I don't get the point of this block (except placing libtinfo.a into deps if
> it is present). Please, clarify it or remove the dead code.
This variables are used by FindCurses module internally. We could set it to 
the libraries we want and FindCurses wouldn't locate dynamic versions for that 
libraries while execution.

> 
> > +if(BUILD_STATIC)
> > +    # Static linking for c++ routines
> > +    add_compile_flags("C;CXX" "-static-libgcc" "-static-libstdc++")
> > +endif()
> > +
> 
Fixed we can't remove libgcc from dynamic dependencies

> You are use `generic_libraries` variable for pthread and dl below. Maybe
> set the variable above and use it here too?
Fixed

> Note: Z_LIBRARY is not used below.
Fixed, Z_LIBRARY is now OPENSSL_LIBRARIES member

> This hunk cannot be applied automatically on top of the current 1.10,
> because of adding ${ICONV_LIBRARIES} in dcac64af. Can you please rebase
> on top of 1.10? The rest are applied successfully.
Fixed

> You have ${generic_libraries} added at end of core.a target, but here it is
Fixed

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