From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-f65.google.com (mail-ot1-f65.google.com [209.85.210.65]) (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 03A0343040C for ; Fri, 28 Aug 2020 00:44:09 +0300 (MSK) Received: by mail-ot1-f65.google.com with SMTP id i11so5711986otr.5 for ; Thu, 27 Aug 2020 14:44:09 -0700 (PDT) MIME-Version: 1.0 References: <20200825140108.52090-1-huston.mavr@gmail.com> <20200825145103.hiqmjkimuk3al3rt@tkn_work_nb> In-Reply-To: <20200825145103.hiqmjkimuk3al3rt@tkn_work_nb> From: Alexandr Barulev Date: Fri, 28 Aug 2020 00:43:56 +0300 Message-ID: Content-Type: multipart/alternative; boundary="000000000000f7f10805ade2d5e2" Subject: Re: [Tarantool-patches] [PATCH v2] build: refactor static build process List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org, yaroslav.dynnikov@gmail.com --000000000000f7f10805ade2d5e2 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hello, thanks for the new iteration of review! I deleted unused box.lua script and reference to it in suite.ini at static-build tests. I added comments about explicitly setting compilers at static-build/CMakeLists.txt. Also I fixed building libncurses/libcurses for correct work of FindCurses.cmake module. After this change there is no need to set CURSES_NEED_NCURSES at cmake/FindReadline.cmake, so I checkouted cmake/FindReadline.cmake from master and update patch commit massage. And fixed .travis.mk static_build_cmake_* jobs - build tarantool with -DCMAKE_BUILD_TYPE=3DRelWithDebInfo cmake option. Here is a new commit message (deleted part about CURSES_NEED_NCURSES): build: refactor static build process Refactored static build process to use static-build/CMakeLists.txt instead of Dockerfile.staticbuild (this allows to support static build on macOS). Following third-party dependencies for static build are installed via cmake `ExternalProject_Add`: - OpenSSL - Zlib - Ncurses - Readline - Unwind - ICU * Added support static build for macOS * Fixed `CONFIGURE_COMMAND` while building bundled libcurl for staic build at file cmake/BuildLibCURL.cmake: - disable building shared libcurl libraries (by setting `--disable-shared` option) - disable hiding libcurl symbols (by setting `--disable-symbol-hiding` option) - prevent linking libcurl with system libz by settign `--with-zlib=3D${FOUND_ZLIB_ROOT_DIR}` option) * Removed Dockerfile.staticbuild * Added new gitlab.ci jobs to test new style static build: - static_build_cmake_linux - static_build_cmake_osx_15 * Removed static_docker_build gitlab.ci job Closes #5095 Here is a link to branch: https://github.com/tarantool/tarantool/tree/rosik/refactor-static-build And here is a diff: diff --git a/.travis.mk b/.travis.mk index 482672429..ccd9d6db1 100644 --- a/.travis.mk +++ b/.travis.mk @@ -151,7 +151,8 @@ test_static_build: deps_debian_static # New static build # builddir used in this target - is a default build path from cmake ExternalProject_Add() test_static_build_cmake_linux: - cd static-build && cmake . && make -j && ctest -V + cd static-build && cmake -DCMAKE_TARANTOOL_ARGS=3D"-DCMAKE_BUILD_TYPE=3DRelWithDebInfo;-DENABLE_WERR= OR=3DON" . && \ + make -j && ctest -V cd test && /usr/bin/python test-run.py --force \ --builddir ${PWD}/static-build/tarantool-prefix/src/tarantool-build $(TEST_RUN_EXTRA_PARAMS) @@ -234,7 +235,8 @@ base_deps_osx: # builddir used in this target - is a default build path from cmake ExternalProject_Add() test_static_build_cmake_osx: base_deps_osx - cd static-build && cmake . && make -j && ctest -V + cd static-build && cmake -DCMAKE_TARANTOOL_ARGS=3D"-DCMAKE_BUILD_TYPE=3DRelWithDebInfo;-DENABLE_WERR= OR=3DON" . && \ + make -j && ctest -V ${INIT_TEST_ENV_OSX}; \ cd test && ./test-run.py --vardir /tmp/tnt \ --builddir ${PWD}/static-build/tarantool-prefix/src/tarantool-build \ diff --git a/cmake/FindReadline.cmake b/cmake/FindReadline.cmake index afe480679..c48bdcb3e 100644 --- a/cmake/FindReadline.cmake +++ b/cmake/FindReadline.cmake @@ -14,17 +14,7 @@ if(BUILD_STATIC) if (NOT CURSES_INFO_LIBRARY) set(CURSES_INFO_LIBRARY "") endif() - - # From Modules/FindCurses.cmake: - # Set ``CURSES_NEED_NCURSES`` to ``TRUE`` before the - # ``find_package(Curses)`` call if NCurses functionality is required. - # This flag is set for linking with required library (installed - # via static-build/CMakeLists.txt). If this variable won't be set - # then tarantool binary links with system library curses which is an - # entire copy of ncurses - set(CURSES_NEED_NCURSES TRUE) endif() - find_package(Curses) if(NOT CURSES_FOUND) find_package(Termcap) diff --git a/static-build/CMakeLists.txt b/static-build/CMakeLists.txt index d90a642e6..ecfdd0455 100644 --- a/static-build/CMakeLists.txt +++ b/static-build/CMakeLists.txt @@ -9,6 +9,12 @@ set(NCURSES_VERSION 6.2) set(READLINE_VERSION 8.0) set(UNWIND_VERSION 1.3-rc1) +# Set compilers explicitly for further configuring dependencies with +# these compilers. This gonna solve libicu building problem in case when +# at dependency configure stage no compiler specified and clang compiler +# exists on linux machine, libicu sources would be compiled with clang +# while for other dependencies (including tarantool) gcc would be used. +# This behaviour causes problem at tarantool linkage stage. if (APPLE) find_program(C_COMPILER clang) find_program(CXX_COMPILER clang++) @@ -70,6 +76,15 @@ ExternalProject_Add(ncurses CXX=3D${CMAKE_CXX_COMPILER} /configure --prefix=3D + + # This flag enables creation of libcurses.a as a symlink to libncurses.a + # and disables subdir creation `ncurses` at /include. It is + # necessary for correct work of FindCurses.cmake module (this module is + # builtin at cmake package) which used in cmake/FindReadline.cmake + --enable-overwrite + --with-termlib # enable building libtinfo to prevent linking + # with libtinfo from system directories + INSTALL_COMMAND ${CMAKE_MAKE_PROGRAM} install.libs ) # diff --git a/static-build/test/static-build/box.lua b/static-build/test/static-build/box.lua deleted file mode 100755 index bad8a9055..000000000 --- a/static-build/test/static-build/box.lua +++ /dev/null @@ -1,3 +0,0 @@ -#!/usr/bin/env tarantool - -require('console').listen(os.getenv('ADMIN')) diff --git a/static-build/test/static-build/suite.ini b/static-build/test/static-build/suite.ini index 92e349466..5aabadd92 100644 --- a/static-build/test/static-build/suite.ini +++ b/static-build/test/static-build/suite.ini @@ -1,5 +1,4 @@ [default] core =3D app description =3D Static build tests -script =3D box.lua is_parallel =3D True =D0=B2=D1=82, 25 =D0=B0=D0=B2=D0=B3. 2020 =D0=B3. =D0=B2 17:51, Alexander T= urenko < alexander.turenko@tarantool.org>: > I looked very briefly (not thoroughly at all) on this iteration. > > There is nothing that confuses me (except few tiny comments below). > > I hope Igor will do thorough review. > > WBR, Alexander Turenko. > > > +if (APPLE) > > + find_program(C_COMPILER clang) > > + find_program(CXX_COMPILER clang++) > > +else() > > + find_program(C_COMPILER gcc) > > + find_program(CXX_COMPILER g++) > > +endif() > > Can we just leave it default? > > In offline discussion Alexandr B. said that tarantool builds with gcc, > but icu with clang that gives some problem. > > Possible solution is to pass ${CMAKE_C_COMPILER} (and CXX too where > necessary) to a subproject as we do for c-ares and curl. It seems it is > already done, so maybe it worth to re-check whether it solves the > problem. > > Anyway, if we really need to set a compiler here explicitly, I don't > mind. Just noted that this way is somewhat unusual as I see. > > > diff --git a/static-build/test/static-build/box.lua > b/static-build/test/static-build/box.lua > > new file mode 100755 > > index 000000000..bad8a9055 > > --- /dev/null > > +++ b/static-build/test/static-build/box.lua > > @@ -0,0 +1,3 @@ > > +#!/usr/bin/env tarantool > > + > > +require('console').listen(os.getenv('ADMIN')) > > Is looks redundant, see the comment below. > > > diff --git a/static-build/test/static-build/suite.ini > b/static-build/test/static-build/suite.ini > > new file mode 100644 > > index 000000000..92e349466 > > --- /dev/null > > +++ b/static-build/test/static-build/suite.ini > > @@ -0,0 +1,5 @@ > > +[default] > > +core =3D app > > +description =3D Static build tests > > +script =3D box.lua > > +is_parallel =3D True > > 'script' does not have sense for 'core =3D app' test, it is for 'core =3D > tarantool' tests. > --000000000000f7f10805ade2d5e2 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello, thanks for the new iteration of re= view!

I deleted unused box.lua script and reference to it in suite.i= ni
at static-build tests. I added comments about explicitly setting
= compilers at static-build/CMakeLists.txt.

Also I fixed building libn= curses/libcurses for correct work of
FindCurses.cmake module. After thi= s change there is no need to set
CURSES_NEED_NCURSES at cmake/FindReadl= ine.cmake, so I checkouted
cmake/FindReadline.cmake from master and upd= ate patch commit massage.

And fixed .tr= avis.mk static_build_cmake_* jobs - build tarantool
with -DCMAKE_BUI= LD_TYPE=3DRelWithDebInfo cmake option.


Here is a new commit mess= age (deleted part about CURSES_NEED_NCURSES):

build: refactor static= build process

Refactored static build process to use static-build/C= MakeLists.txt
instead of Dockerfile.staticbuild (this allows to support = static
build on macOS). Following third-party dependencies for static bu= ild
are installed via cmake `ExternalProject_Add`:
=C2=A0 =C2=A0 - Op= enSSL
=C2=A0 =C2=A0 - Zlib
=C2=A0 =C2=A0 - Ncurses
=C2=A0 =C2=A0 -= Readline
=C2=A0 =C2=A0 - Unwind
=C2=A0 =C2=A0 - ICU

* Added s= upport static build for macOS
* Fixed `CONFIGURE_COMMAND` while building= bundled libcurl for staic
=C2=A0 =C2=A0 build at file cmake/BuildLibCUR= L.cmake:
=C2=A0 =C2=A0 - disable building shared libcurl libraries (by s= etting
=C2=A0 =C2=A0 =C2=A0 =C2=A0 `--disable-shared` option)
=C2=A0 = =C2=A0 - disable hiding libcurl symbols (by setting
=C2=A0 =C2=A0 =C2=A0= =C2=A0 `--disable-symbol-hiding` option)
=C2=A0 =C2=A0 - prevent linkin= g libcurl with system libz by settign
=C2=A0 =C2=A0 =C2=A0 =C2=A0 `--wit= h-zlib=3D${FOUND_ZLIB_ROOT_DIR}` option)
* Removed Dockerfile.staticbuil= d
* Added new gitlab.ci jobs to test ne= w style static build:
=C2=A0 =C2=A0 - static_build_cmake_linux
=C2=A0= =C2=A0 - static_build_cmake_osx_15
* Removed static_docker_build gitlab.ci job

Closes #5095



An= d here is a diff:

=C2=A0diff --git a/.t= ravis.mk b/.travis.mk
index 4826724= 29..ccd9d6db1 100644
--- a/.travis.mk+++ b/.travis.mk
@@ -151,7 +151,8 @@ = test_static_build: deps_debian_static
=C2=A0# New static build
=C2=A0= # builddir used in this target - is a default build path from cmake Externa= lProject_Add()
=C2=A0test_static_build_cmake_linux:
- cd static-build= && cmake . && make -j && ctest -V
+ cd static-b= uild && cmake -DCMAKE_TARANTOOL_ARGS=3D"-DCMAKE_BUILD_TYPE=3DR= elWithDebInfo;-DENABLE_WERROR=3DON" . && \
+ make -j &&= amp; ctest -V
=C2=A0 cd test && /usr/bin/python test-run.py --fo= rce \
=C2=A0 --builddir ${PWD}/static-build/tarantool-prefix/src/tarant= ool-build $(TEST_RUN_EXTRA_PARAMS)
=C2=A0
@@ -234,7 +235,8 @@ base_de= ps_osx:
=C2=A0
=C2=A0# builddir used in this target - is a default bu= ild path from cmake ExternalProject_Add()
=C2=A0test_static_build_cmake_= osx: base_deps_osx
- cd static-build && cmake . && make = -j && ctest -V
+ cd static-build && cmake -DCMAKE_TARANT= OOL_ARGS=3D"-DCMAKE_BUILD_TYPE=3DRelWithDebInfo;-DENABLE_WERROR=3DON&q= uot; . && \
+ make -j && ctest -V
=C2=A0 ${INIT_TEST_= ENV_OSX}; \
=C2=A0 cd test && ./test-run.py --vardir /tmp/tnt \<= br>=C2=A0 --builddir ${PWD}/static-build/tarantool-prefix/src/tarantool-bu= ild \
diff --git a/cmake/FindReadline.cmake b/cmake/FindReadline.cmakeindex afe480679..c48bdcb3e 100644
--- a/cmake/FindReadline.cmake
++= + b/cmake/FindReadline.cmake
@@ -14,17 +14,7 @@ if(BUILD_STATIC)
=C2= =A0 =C2=A0 =C2=A0if (NOT CURSES_INFO_LIBRARY)
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0set(CURSES_INFO_LIBRARY "")
=C2=A0 =C2=A0 =C2=A0endi= f()
-
- =C2=A0 =C2=A0# From Modules/FindCurses.cmake:
- =C2=A0 =C2= =A0# Set ``CURSES_NEED_NCURSES`` to ``TRUE`` before the
- =C2=A0 =C2=A0#= ``find_package(Curses)`` call if NCurses functionality is required.
- = =C2=A0 =C2=A0# This flag is set for linking with required library (installe= d
- =C2=A0 =C2=A0# via static-build/CMakeLists.txt). If this variable wo= n't be set
- =C2=A0 =C2=A0# then tarantool binary links with system = library curses which is an
- =C2=A0 =C2=A0# entire copy of ncurses
- = =C2=A0 =C2=A0set(CURSES_NEED_NCURSES TRUE)
=C2=A0endif()
-
=C2=A0f= ind_package(Curses)
=C2=A0if(NOT CURSES_FOUND)
=C2=A0 =C2=A0 =C2=A0fi= nd_package(Termcap)
diff --git a/static-build/CMakeLists.txt b/static-bu= ild/CMakeLists.txt
index d90a642e6..ecfdd0455 100644
--- a/static-bui= ld/CMakeLists.txt
+++ b/static-build/CMakeLists.txt
@@ -9,6 +9,12 @@ = set(NCURSES_VERSION 6.2)
=C2=A0set(READLINE_VERSION 8.0)
=C2=A0set(UN= WIND_VERSION 1.3-rc1)
=C2=A0
+# Set compilers explicitly for further = configuring dependencies with
+# these compilers. This gonna solve libic= u building problem in case when
+# at dependency configure stage no comp= iler specified and clang compiler
+# exists on linux machine, libicu sou= rces would be compiled with clang
+# while for other dependencies (inclu= ding tarantool) gcc would be used.
+# This behaviour causes problem at t= arantool linkage stage.
=C2=A0if (APPLE)
=C2=A0 =C2=A0 =C2=A0find_pro= gram(C_COMPILER clang)
=C2=A0 =C2=A0 =C2=A0find_program(CXX_COMPILER cla= ng++)
@@ -70,6 +76,15 @@ ExternalProject_Add(ncurses
=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0CXX=3D${CMAKE_CXX_COMPILER}
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0<SOURCE_DIR>/configure
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0--prefix=3D<INSTALL_DIR>
+
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0# Th= is flag enables creation of libcurses.a as a symlink to libncurses.a
+ = =C2=A0 =C2=A0 =C2=A0 =C2=A0# and disables subdir creation `ncurses` at =C2= =A0<install_dir>/include. It is
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0# nec= essary for correct work of FindCurses.cmake module (this module is
+ =C2= =A0 =C2=A0 =C2=A0 =C2=A0# builtin at cmake package) which used in cmake/Fin= dReadline.cmake
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0--enable-overwrite
+ =C2= =A0 =C2=A0 =C2=A0 =C2=A0--with-termlib =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0# = enable building libtinfo to prevent linking
+ =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0# with libtinfo from system directories
+ =C2=A0 =C2=A0INST= ALL_COMMAND ${CMAKE_MAKE_PROGRAM} install.libs
=C2=A0)
=C2=A0
=C2= =A0#
diff --git a/static-build/test/static-build/box.lua b/static-build/= test/static-build/box.lua
deleted file mode 100755
index bad8a9055..0= 00000000
--- a/static-build/test/static-build/box.lua
+++ /dev/null@@ -1,3 +0,0 @@
-#!/usr/bin/env tarantool
-
-require('consol= e').listen(os.getenv('ADMIN'))
diff --git a/static-build/tes= t/static-build/suite.ini b/static-build/test/static-build/suite.ini
inde= x 92e349466..5aabadd92 100644
--- a/static-build/test/static-build/suite= .ini
+++ b/static-build/test/static-build/suite.ini
@@ -1,5 +1,4 @@=C2=A0[default]
=C2=A0core =3D app
=C2=A0description =3D Static bui= ld tests
-script =3D box.lua
=C2=A0is_parallel =3D True

=
=D0=B2=D1= =82, 25 =D0=B0=D0=B2=D0=B3. 2020 =D0=B3. =D0=B2 17:51, Alexander Turenko &l= t;alexander.turenko@tara= ntool.org>:
I looked very briefly (not thoroughly at all) on this iteration.

There is nothing that confuses me (except few tiny comments below).

I hope Igor will do thorough review.

WBR, Alexander Turenko.

> +if (APPLE)
> +=C2=A0 =C2=A0 find_program(C_COMPILER clang)
> +=C2=A0 =C2=A0 find_program(CXX_COMPILER clang++)
> +else()
> +=C2=A0 =C2=A0 find_program(C_COMPILER gcc)
> +=C2=A0 =C2=A0 find_program(CXX_COMPILER g++)
> +endif()

Can we just leave it default?

In offline discussion Alexandr B. said that tarantool builds with gcc,
but icu with clang that gives some problem.

Possible solution is to pass ${CMAKE_C_COMPILER} (and CXX too where
necessary) to a subproject as we do for c-ares and curl. It seems it is
already done, so maybe it worth to re-check whether it solves the
problem.

Anyway, if we really need to set a compiler here explicitly, I don't mind. Just noted that this way is somewhat unusual as I see.

> diff --git a/static-build/test/static-build/box.lua b/static-build/tes= t/static-build/box.lua
> new file mode 100755
> index 000000000..bad8a9055
> --- /dev/null
> +++ b/static-build/test/static-build/box.lua
> @@ -0,0 +1,3 @@
> +#!/usr/bin/env tarantool
> +
> +require('console').listen(os.getenv('ADMIN'))

Is looks redundant, see the comment below.

> diff --git a/static-build/test/static-build/suite.ini b/static-build/t= est/static-build/suite.ini
> new file mode 100644
> index 000000000..92e349466
> --- /dev/null
> +++ b/static-build/test/static-build/suite.ini
> @@ -0,0 +1,5 @@
> +[default]
> +core =3D app
> +description =3D Static build tests
> +script =3D box.lua
> +is_parallel =3D True

'script' does not have sense for 'core =3D app' test, it is= for 'core =3D
tarantool' tests.
--000000000000f7f10805ade2d5e2--