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

["Alexander V. Tikhonov" <avtikhon@tarantool.org>]

Hi Alexander, thanks a lot for the deep review, I've made all
your suggestions.

On Sun, Jul 05, 2020 at 03:37:36AM +0300, Alexander Turenko wrote:
> 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.
>

Right, removed.

> > Implemented build with testing for opensuse images:
> > SuSE 15.0, 15.1, 15.2
> 
> Nit: SuSE **Leap** 15.0, 15.1, 15.2.
>

Corrected.

> 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.
>

Ok.

> > 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.
> 

Right, I'd like to save the link on %{sle_version} for fast search.

> > 
> > 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.
> 

Sure, I've updated it as suggested.

> > 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'.)
> 

Ok, I've used your suggested macro definition and all my changes below
became unneeded.

> >  %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.
> 

Right not need any more - removed.

> > +%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.
> 

Right not need any more - removed.

> >  %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.
> 

Right not need any more - removed.

> > 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}")
> 

Sure, I've changed it as you suggested.

> 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
>

Sure, updated too.

> [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.
> 

Right, added.

> 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.

Updated issue, added comment to commit message.