[Tarantool-patches] [PATCH v1] build: implement SuSE build with testing

[Alexander Turenko]

On Thu, Apr 23, 2020 at 12:03:44PM +0300, Alexander V. Tikhonov wrote:
> Added makefile to build the SuSE based packages.

Makefile? Is is about packpack update? It is not actual anymore (since
we decided to update existing makefile). But I would rather just remove
the comment, because it is about packpack patch, not this one.

> Implemented build with testing for opensuse images:
> SuSE 15.0, 15.1, 15.2

Nit: SuSE **Leap** 15.0, 15.1, 15.2.

Are we plan to deploy them to our repositories? I guess a user who asked
for this expects that we'll offer those packages like other ones.

Ouch, I see, the branch already contains deployment rules. So this
comment is not actual anymore.

> Tarantool spec file changed with:
> Added checks of %{sle_version} according to
> https://en.opensuse.org/openSUSE:Packaging_for_Leap#RPM_Distro_Version_Macros

Nit: I think that using of %{sle_version} macro is quite obvious and
does not require a comment here. But up to you.

> 
> Found that opensuse adding linker flag like '--no-undefined' which
> produces the fails on building test modules at app-tap/app/box-tap
> suites. Decided to block this flag.
> 
> Running tests by make test, because opensuse uses out-of-source
> build by default.
> Set complete testing except replication suite.
> Test box-tap/cfg fails and temporary blocked according to the
> issue created for it #4594.

Don't forget to update those explanations according to everything said
below.

> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4562-suse-pack-full-ci
> Issue: https://github.com/tarantool/tarantool/issues/4562

> diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
> index ff95ed646..09de015e2 100644
> --- a/rpm/tarantool.spec
> +++ b/rpm/tarantool.spec

I give some recommendations how to better handle out-of-source build in
different places, but now another idea comes into my mind. We can just
define `%global %__builddir .`, that's all. (Don't forget to leave a
comment for it in the spec, like 'Force in-source build on openSUSE to
simplify the RPM spec'.)

>  %build
>  # RHBZ #1301720: SYSCONFDIR an LOCALSTATEDIR must be specified explicitly
> -%cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo \
> +%if (0%{?sle_version} >= 1500)
> +# SuSE uses only out-of-source builds in its macros
> +%cmake .. \
> +         -DCMAKE_INSTALL_PREFIX:PATH=/usr \
> +%else
> +%cmake . \
> +%endif
> +	 -DCMAKE_BUILD_TYPE=RelWithDebInfo \
>           -DCMAKE_INSTALL_LOCALSTATEDIR:PATH=%{_localstatedir} \
>           -DCMAKE_INSTALL_SYSCONFDIR:PATH=%{_sysconfdir} \

It would look better I think if we'll just use '%cmake
%{_sourcedir} <...>'. It seems it should work with packpack.

It seems -DCMAKE_INSTALL_PREFIX:PATH=/usr is not needed, %cmake macro
already has it on opensuse-leap-15.2.

But I hope that %__builddir redefinition will work, it would allow to
kepp the RPM spec almost same for different distros.

> +%if (0%{?sle_version} == 0)
>  %install
> +%endif
>  %make_install

Do you want to do `make install` in the %build phase rather than
%install? It feels as not right way to overcome a problem.

I don't see anything that explains this change in the commit message or
in a comment here. How should I review it? Build myself, see a result
and try to guess the reason? It would be the waste of time: you already
did this and could explain.

I have a guess! Is it to keep CWD after 'cd build' in %cmake expansion?
If so, you can just use %cmake_install when it is defined and
%make_install otherwise.

But I hope that redefinition of %__builddir will work.

>  %check
> -%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7)
> +%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7 || 0%{?sle_version} >= 1500)
>  # https://github.com/tarantool/tarantool/issues/1227
>  echo "self.skip = True" > ./test/app/socket.skipcond
>  # https://github.com/tarantool/tarantool/issues/1322
>  echo "self.skip = True" > ./test/app/digest.skipcond
>  # run a safe subset of the test suite
> +%if (0%{?sle_version} >= 1500)
> +# SuSE uses only out-of-source builds always at build/ subdir in its macros
> +cd build && TEST_RUN_EXCLUDE='replication/' make test-force
> +%else
>  cd test && ./test-run.py --force -j 1 unit/ app/ app-tap/ box/ box-tap/ engine/ vinyl/
>  %endif
> +%endif

First, this hunk is different now and the branch should be rebased on
top of current master.

Second, it is better to avoid duplication and use something like:

 | %if 0%{?__builddir:1}
 | cd %__builddir
 | %endif

But I hope %__builddir redefinition will work.

> diff --git a/test/app-tap/CMakeLists.txt b/test/app-tap/CMakeLists.txt
> index ee67cf533..68aa3690b 100644
> --- a/test/app-tap/CMakeLists.txt
> +++ b/test/app-tap/CMakeLists.txt
> @@ -1 +1,2 @@
> +SET(CMAKE_SHARED_LINKER_FLAGS "")

Nit: We usually type 'set()' lowercased in our sources.

After the first glance I was sceptic about redefinition of a cmake
variable that may affect build rules for the whole project. But then I
read about CMake scopes and found that the change affects only this
directory, because add_subdirectory() creates a new variable scope.
Then I asked myself, whether it applicable for cached variables
(CMAKE_SHARED_LINKER_FLAGS is cached). And found that set() will create
a new non-cached variable, which will be used directory-wide instead of
global one and will not affect the global cached value (see [1]). So it
is okay.

But we can do it less intruisive:

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

Also I think it should be properly commented, like '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'.

BTW, you should also update luajit submodule in the same way; new tests
are added here:

- test/gh-4427-ffi-sandwich/CMakeLists.txt
- test/lj-flush-on-trace/CMakeLists.txt

[1]: https://cmake.org/cmake/help/v3.0/command/set.html

> diff --git a/test/box-tap/cfg.skipcond b/test/box-tap/cfg.skipcond
> index 7950a5d93..9d5535f87 100644
> --- a/test/box-tap/cfg.skipcond
> +++ b/test/box-tap/cfg.skipcond
> @@ -1,8 +1,14 @@
> +import os
>  import platform
>  
>  # Disabled on FreeBSD due to fail #4271:
>  #   Data segment size exceeds process limit
>  if platform.system() == 'FreeBSD':
>      self.skip = 1
> +try:
> +    if os.popen('lsb_release -i').read().split(':')[1].strip() == 'openSUSE':
> +        self.skip = 1
> +except IndexError:
> +    pass
>  
>  # vim: set ft=python:

You should place the issue number (#4594) here.

Neither this file, nor the issue give any idea what is wrong with the
test. We cannot disable a test just because it fails: why we writing
them so? We should at least ensure that it is the testing problem and we
can provide openSUSE packages and say that tarantool works on openSUSE
without critical problems.

The test is disabled on FreeBSD and here there is the certain reason. It
looks like related to os.execute() and so we can state something about
FreeBSD support in tarantool: it works, but there are problems around
forking processes from tarantool.