[Tarantool-patches] [PATCH v2 0/3] gitlab-ci: add out-of-source build

[Tarantool-patches] [PATCH v2 0/3] gitlab-ci: add out-of-source build

[Alexander V. Tikhonov]

gitlab-ci: add out-of-source build
    
Implemented out-of-source build at cmake files.
Added out of source build make targets and added
test job to gitlab-ci.
 
Skipped failing test app-tap/http_client.test.lua
from out-of-source build. Left #4258 issue opened
till it will be fixed.
    
Temporary added branch in curl repository
with fix for out-of-source build.
    
Part of #4258

---

V1: https://lists.tarantool.org/pipermail/tarantool-patches/2019-October/012004.html

Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4874-out-of-source-build-full-ci
Issue: https://github.com/tarantool/tarantool/issues/4258

Alexander V. Tikhonov (3):
  Fix curl repository for out-of-source build
  gitlab-ci: add out-of-source build
  Skip failing test from out-of-source build

 .gitlab-ci.yml                    |  7 +++++++
 .travis.mk                        | 18 ++++++++++++++++++
 cmake/BuildLibCURL.cmake          |  6 +++---
 cmake/utils.cmake                 |  4 ++--
 src/box/CMakeLists.txt            |  3 ++-
 test/app-tap/http_client.skipcond |  5 +++++
 third_party/curl                  |  2 +-
 
7 files changed, 38 insertions(+), 7 deletions(-)

-- 
2.17.1

[Tarantool-patches] [PATCH v2 1/3] Fix curl repository for out-of-source build

[Alexander V. Tikhonov]

Temporary added branch in curl repository
with fix for out-of-source build.

Part of #4258
---
 third_party/curl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/third_party/curl b/third_party/curl
index 9cd755e1d..bf6df48c8 160000
--- a/third_party/curl
+++ b/third_party/curl
@@ -1 +1 @@
-Subproject commit 9cd755e1d768bbf228e7c9faf223b7459f7e0105
+Subproject commit bf6df48c8c9b8377f71c2a09ff545d828c7ed2fb
-- 
2.17.1

[Tarantool-patches] [PATCH v2 2/3] gitlab-ci: add out-of-source build

[Alexander V. Tikhonov]

Implemented out-of-source build at cmake files.
Added out of source build make targets and added
test job to gitlab-ci.

Part of #4258
---
 .gitlab-ci.yml           |  7 +++++++
 .travis.mk               | 18 ++++++++++++++++++
 cmake/BuildLibCURL.cmake |  6 +++---
 cmake/utils.cmake        |  4 ++--
 src/box/CMakeLists.txt   |  3 ++-
 5 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index fd265aa60..cbf02f8f5 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -98,6 +98,13 @@ variables:
 
 # Tests
 
+out_of_source:
+  stage: test
+  tags:
+    - deploy_test
+  script:
+    - docker run -w /tmp/ro_src --mount type=bind,source="${PWD}",target=/tmp/ro_src,readonly,bind-propagation=rslave -i registry.gitlab.com/tarantool/tarantool/testing/debian-stretch:latest make -f /tmp/ro_src/.travis.mk test_outofsrc_no_deps
+
 release:
   <<: *docker_test_definition
   script:
diff --git a/.travis.mk b/.travis.mk
index 8aa0e6116..5cde9d22e 100644
--- a/.travis.mk
+++ b/.travis.mk
@@ -6,6 +6,8 @@ DOCKER_IMAGE?=packpack/packpack:debian-stretch
 TEST_RUN_EXTRA_PARAMS?=
 MAX_FILES?=65534
 MAX_PROC?=2500
+STARTED_AT_PATH=$(shell pwd)
+OUT_OF_SRC_BLD_PATH=/tmp/rw_bins
 
 all: package
 
@@ -74,6 +76,22 @@ deps_buster_clang_8: deps_debian
 	apt-get update
 	apt-get install -y clang-8 llvm-8-dev
 
+# Out-of-source build
+
+build_outofsrc:
+	rm -rf ${OUT_OF_SRC_BLD_PATH} && \
+		mkdir ${OUT_OF_SRC_BLD_PATH} && \
+		cd ${OUT_OF_SRC_BLD_PATH} && \
+		cmake ${STARTED_AT_PATH} \
+			-DCMAKE_BUILD_TYPE=RelWithDebInfo \
+			-DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS} && \
+		make -j
+
+test_outofsrc_no_deps: build_outofsrc
+	cd ${OUT_OF_SRC_BLD_PATH} && make test-force
+
+test_outofsrc: deps_debian test_outofsrc_no_deps
+
 # Release
 
 build_debian:
diff --git a/cmake/BuildLibCURL.cmake b/cmake/BuildLibCURL.cmake
index 5f8b15a63..753b7293a 100644
--- a/cmake/BuildLibCURL.cmake
+++ b/cmake/BuildLibCURL.cmake
@@ -56,10 +56,10 @@ macro(curl_build)
         DOWNLOAD_DIR ${LIBCURL_BINARY_DIR}
         TMP_DIR ${LIBCURL_BINARY_DIR}/tmp
         STAMP_DIR ${LIBCURL_BINARY_DIR}/stamp
-        BINARY_DIR ${LIBCURL_BINARY_DIR}
+        BINARY_DIR ${LIBCURL_BINARY_DIR}/curl
         CONFIGURE_COMMAND
-            cd <SOURCE_DIR> && ./buildconf &&
-            cd <BINARY_DIR> && <SOURCE_DIR>/configure
+            rm -rf <BINARY_DIR> && cp -rf <SOURCE_DIR> <BINARY_DIR> &&
+            cd <BINARY_DIR> && ./buildconf && ./configure
                 # Pass the same toolchain as is used to build
                 # tarantool itself, because they can be
                 # incompatible.
diff --git a/cmake/utils.cmake b/cmake/utils.cmake
index 3ab2d3ff2..6044ea7c0 100644
--- a/cmake/utils.cmake
+++ b/cmake/utils.cmake
@@ -70,9 +70,9 @@ endfunction()
 
 function(bin_source varname srcfile dstfile)
     set(var ${${varname}})
-    set(${varname} ${var} ${dstfile} PARENT_SCOPE)
+    set(${varname} ${var} "${CMAKE_BINARY_DIR}/${dstfile}" PARENT_SCOPE)
     set (srcfile "${CMAKE_CURRENT_SOURCE_DIR}/${srcfile}")
-    set (dstfile "${CMAKE_CURRENT_SOURCE_DIR}/${dstfile}")
+    set (dstfile "${CMAKE_BINARY_DIR}/${dstfile}")
     set (tmpfile "${dstfile}.tmp")
     get_filename_component(module ${dstfile} NAME_WE)
 
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index c931ecdfe..51b0006b1 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -23,7 +23,7 @@ lua_source(lua_sources lua/xlog.lua)
 lua_source(lua_sources lua/key_def.lua)
 lua_source(lua_sources lua/merger.lua)
 set(bin_sources)
-bin_source(bin_sources bootstrap.snap bootstrap.h)
+bin_source(bin_sources bootstrap.snap src/box/bootstrap.h)
 
 add_custom_target(box_generate_lua_sources
     WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/src/box
@@ -32,6 +32,7 @@ set_property(DIRECTORY PROPERTY ADDITIONAL_MAKE_CLEAN_FILES ${lua_sources})
 
 include_directories(${ZSTD_INCLUDE_DIRS})
 include_directories(${CMAKE_BINARY_DIR}/src/box/sql)
+include_directories(${CMAKE_BINARY_DIR}/src/box)
 
 add_library(box_error STATIC error.cc errcode.c vclock.c mp_error.cc)
 target_link_libraries(box_error core stat mpstream)
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v2 2/3] gitlab-ci: add out-of-source build

[Alexander Turenko]

> index 5f8b15a63..753b7293a 100644
> --- a/cmake/BuildLibCURL.cmake
> +++ b/cmake/BuildLibCURL.cmake
> @@ -56,10 +56,10 @@ macro(curl_build)
>          DOWNLOAD_DIR ${LIBCURL_BINARY_DIR}
>          TMP_DIR ${LIBCURL_BINARY_DIR}/tmp
>          STAMP_DIR ${LIBCURL_BINARY_DIR}/stamp
> -        BINARY_DIR ${LIBCURL_BINARY_DIR}
> +        BINARY_DIR ${LIBCURL_BINARY_DIR}/curl

Don't get what is the problem you try to solve here.

>          CONFIGURE_COMMAND
> -            cd <SOURCE_DIR> && ./buildconf &&
> -            cd <BINARY_DIR> && <SOURCE_DIR>/configure
> +            rm -rf <BINARY_DIR> && cp -rf <SOURCE_DIR> <BINARY_DIR> &&
> +            cd <BINARY_DIR> && ./buildconf && ./configure

Will not it lead to full curl rebuild on each `make` run when something
changed within the repo?

>                  # Pass the same toolchain as is used to build
>                  # tarantool itself, because they can be
>                  # incompatible.
> diff --git a/cmake/utils.cmake b/cmake/utils.cmake
> index 3ab2d3ff2..6044ea7c0 100644
> --- a/cmake/utils.cmake
> +++ b/cmake/utils.cmake
> @@ -70,9 +70,9 @@ endfunction()
>  
>  function(bin_source varname srcfile dstfile)
>      set(var ${${varname}})
> -    set(${varname} ${var} ${dstfile} PARENT_SCOPE)
> +    set(${varname} ${var} "${CMAKE_BINARY_DIR}/${dstfile}" PARENT_SCOPE)

It is better to sink this line down below set(dstfile, ...).

>      set (srcfile "${CMAKE_CURRENT_SOURCE_DIR}/${srcfile}")
> -    set (dstfile "${CMAKE_CURRENT_SOURCE_DIR}/${dstfile}")
> +    set (dstfile "${CMAKE_BINARY_DIR}/${dstfile}")

Why not CMAKE_CURRENT_BINARY_DIR? Now path to a source file is
calculated relative to a currently processed directory, but path to a
destination file is relative to a project binary directory that looks
lopsided.

> diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
> index c931ecdfe..51b0006b1 100644
> --- a/src/box/CMakeLists.txt
> +++ b/src/box/CMakeLists.txt
> @@ -23,7 +23,7 @@ lua_source(lua_sources lua/xlog.lua)
>  lua_source(lua_sources lua/key_def.lua)
>  lua_source(lua_sources lua/merger.lua)
>  set(bin_sources)
> -bin_source(bin_sources bootstrap.snap bootstrap.h)
> +bin_source(bin_sources bootstrap.snap src/box/bootstrap.h)

Note: I guess it will not be needed if we'll use
CMAKE_CURRENT_BINARY_DIR in bin_source() function.

>  
>  add_custom_target(box_generate_lua_sources
>      WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/src/box
> @@ -32,6 +32,7 @@ set_property(DIRECTORY PROPERTY ADDITIONAL_MAKE_CLEAN_FILES ${lua_sources})
>  
>  include_directories(${ZSTD_INCLUDE_DIRS})
>  include_directories(${CMAKE_BINARY_DIR}/src/box/sql)
> +include_directories(${CMAKE_BINARY_DIR}/src/box)

Generating of bootstrap.h into a binary directory is the nice change!

Re: [Tarantool-patches] [PATCH v2 2/3] gitlab-ci: add out-of-source build

[Alexander V. Tikhonov]

Hi Alexander, thanks a lot for the review, please check my comments below.

On Sat, Apr 25, 2020 at 10:17:30AM +0300, Alexander Turenko wrote:
> > index 5f8b15a63..753b7293a 100644
> > --- a/cmake/BuildLibCURL.cmake
> > +++ b/cmake/BuildLibCURL.cmake
> > @@ -56,10 +56,10 @@ macro(curl_build)
> >          DOWNLOAD_DIR ${LIBCURL_BINARY_DIR}
> >          TMP_DIR ${LIBCURL_BINARY_DIR}/tmp
> >          STAMP_DIR ${LIBCURL_BINARY_DIR}/stamp
> > -        BINARY_DIR ${LIBCURL_BINARY_DIR}
> > +        BINARY_DIR ${LIBCURL_BINARY_DIR}/curl
> 
> Don't get what is the problem you try to solve here.
> 

The path to curl binaries is set here, which later used
for configuring/building/installing and no need to write
it there each time.

> >          CONFIGURE_COMMAND
> > -            cd <SOURCE_DIR> && ./buildconf &&
> > -            cd <BINARY_DIR> && <SOURCE_DIR>/configure
> > +            rm -rf <BINARY_DIR> && cp -rf <SOURCE_DIR> <BINARY_DIR> &&
> > +            cd <BINARY_DIR> && ./buildconf && ./configure
> 
> Will not it lead to full curl rebuild on each `make` run when something
> changed within the repo?

Right, I've changed it to cmake in separate commit with issue #4968,
to be able to use Out-Of-Source build.

> 
> >                  # Pass the same toolchain as is used to build
> >                  # tarantool itself, because they can be
> >                  # incompatible.
> > diff --git a/cmake/utils.cmake b/cmake/utils.cmake
> > index 3ab2d3ff2..6044ea7c0 100644
> > --- a/cmake/utils.cmake
> > +++ b/cmake/utils.cmake
> > @@ -70,9 +70,9 @@ endfunction()
> >  
> >  function(bin_source varname srcfile dstfile)
> >      set(var ${${varname}})
> > -    set(${varname} ${var} ${dstfile} PARENT_SCOPE)
> > +    set(${varname} ${var} "${CMAKE_BINARY_DIR}/${dstfile}" PARENT_SCOPE)
> 
> It is better to sink this line down below set(dstfile, ...).
> 

Sure, corrected, as suggested.

> >      set (srcfile "${CMAKE_CURRENT_SOURCE_DIR}/${srcfile}")
> > -    set (dstfile "${CMAKE_CURRENT_SOURCE_DIR}/${dstfile}")
> > +    set (dstfile "${CMAKE_BINARY_DIR}/${dstfile}")
> 
> Why not CMAKE_CURRENT_BINARY_DIR? Now path to a source file is
> calculated relative to a currently processed directory, but path to a
> destination file is relative to a project binary directory that looks
> lopsided.

Sure, corrected, as suggested.

> 
> > diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
> > index c931ecdfe..51b0006b1 100644
> > --- a/src/box/CMakeLists.txt
> > +++ b/src/box/CMakeLists.txt
> > @@ -23,7 +23,7 @@ lua_source(lua_sources lua/xlog.lua)
> >  lua_source(lua_sources lua/key_def.lua)
> >  lua_source(lua_sources lua/merger.lua)
> >  set(bin_sources)
> > -bin_source(bin_sources bootstrap.snap bootstrap.h)
> > +bin_source(bin_sources bootstrap.snap src/box/bootstrap.h)
> 
> Note: I guess it will not be needed if we'll use
> CMAKE_CURRENT_BINARY_DIR in bin_source() function.
> 

Right, after that correction no need in this change.

> >  
> >  add_custom_target(box_generate_lua_sources
> >      WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/src/box
> > @@ -32,6 +32,7 @@ set_property(DIRECTORY PROPERTY ADDITIONAL_MAKE_CLEAN_FILES ${lua_sources})
> >  
> >  include_directories(${ZSTD_INCLUDE_DIRS})
> >  include_directories(${CMAKE_BINARY_DIR}/src/box/sql)
> > +include_directories(${CMAKE_BINARY_DIR}/src/box)
> 
> Generating of bootstrap.h into a binary directory is the nice change!

Right, temporary change, forgot to remove.

Re: [Tarantool-patches] [PATCH v2 2/3] gitlab-ci: add out-of-source build

[Alexander Turenko]

While we discussing curl and so, there is clear fix re bootstrap.h. I'll
not mind if you'll send it within a separate patch.

On Wed, May 13, 2020 at 10:40:00AM +0300, Alexander V. Tikhonov wrote:
> Hi Alexander, thanks a lot for the review, please check my comments below.
> 
> On Sat, Apr 25, 2020 at 10:17:30AM +0300, Alexander Turenko wrote:
> > > index 5f8b15a63..753b7293a 100644
> > > --- a/cmake/BuildLibCURL.cmake
> > > +++ b/cmake/BuildLibCURL.cmake
> > > @@ -56,10 +56,10 @@ macro(curl_build)
> > >          DOWNLOAD_DIR ${LIBCURL_BINARY_DIR}
> > >          TMP_DIR ${LIBCURL_BINARY_DIR}/tmp
> > >          STAMP_DIR ${LIBCURL_BINARY_DIR}/stamp
> > > -        BINARY_DIR ${LIBCURL_BINARY_DIR}
> > > +        BINARY_DIR ${LIBCURL_BINARY_DIR}/curl
> > 
> > Don't get what is the problem you try to solve here.
> > 
> 
> The path to curl binaries is set here, which later used
> for configuring/building/installing and no need to write
> it there each time.

I'll try to ask it in other words. What is wrong with
LIBCURL_BINARY_DIR, which is ${PROJECT_BINARY_DIR}/build/curl/work now?
Okay, if something wrong, then shouldn't we set it to
${PROJECT_BINARY_DIR}/build/curl/work/curl right at initialization
(however I don't see a reason)?

Is not the reason https://github.com/curl/curl/pull/5213 ? Can we avoid
this change if we'll update the curl submodule?

WBR, Alexander TUrenko.

[Tarantool-patches] [PATCH v2 3/3] Skip failing test from out-of-source build

[Alexander V. Tikhonov]

Skipped failing test app-tap/http_client.test.lua
from out-of-source build. Left #4258 issue opened
till it will be fixed.

Part of #4258
---
 .travis.mk                        | 2 +-
 test/app-tap/http_client.skipcond | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/.travis.mk b/.travis.mk
index 5cde9d22e..978be14ab 100644
--- a/.travis.mk
+++ b/.travis.mk
@@ -88,7 +88,7 @@ build_outofsrc:
 		make -j
 
 test_outofsrc_no_deps: build_outofsrc
-	cd ${OUT_OF_SRC_BLD_PATH} && make test-force
+	cd ${OUT_OF_SRC_BLD_PATH} && OUTOFSRC_BUILD=ON make test-force
 
 test_outofsrc: deps_debian test_outofsrc_no_deps
 
diff --git a/test/app-tap/http_client.skipcond b/test/app-tap/http_client.skipcond
index 48e17903e..2cc2572c5 100644
--- a/test/app-tap/http_client.skipcond
+++ b/test/app-tap/http_client.skipcond
@@ -1,5 +1,10 @@
+import os
 import platform
 
+# Disabled at out-of-source build due to issue #4258.
+if os.getenv("OUTOFSRC_BUILD") == 'ON':
+    self.skip = 1
+
 # Disabled on FreeBSD due to flaky fail #4271.
 if platform.system() == 'FreeBSD':
     self.skip = 1
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v2 3/3] Skip failing test from out-of-source build

[Alexander Turenko]

On Wed, Apr 22, 2020 at 08:44:45PM +0300, Alexander V. Tikhonov wrote:
> Skipped failing test app-tap/http_client.test.lua

What is the reason? Why tarantool is different at all depending of a way
to build? Or it is something with testing system?

Please, provide some details. Or, if it is something in testing system
like 'X not going to be copied to Y', then, maybe, fix it right here?

> from out-of-source build. Left #4258 issue opened
> till it will be fixed.
> 
> Part of #4258