Re: [Tarantool-patches] [PATCH v2] gitlab-ci: add openSuSE packages build jobs

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

Alexander, thanks for your new review, I thought about all topics you
mentioned, but guessed the wrong way as I see now.

On Mon, Jul 06, 2020 at 09:36:07PM +0300, Alexander Turenko wrote:
> > 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/prebuild-opensuse-leap.sh b/rpm/prebuild-opensuse-leap.sh
> > new file mode 100755
> > index 000000000..0782ebd0d
> > --- /dev/null
> > +++ b/rpm/prebuild-opensuse-leap.sh
> > @@ -0,0 +1,7 @@
> > +#!/bin/bash -ex
> 
> Nit: There is nothing bash specific here. I prefer to use `#!/bin/sh`
> shebang for such scripts.
>

Thought that use by default bash if it exists by default is much
better. Anyway changed it.

> > +
> > +# Found that packages repositories can have broken
> > +# repositories in zypper cache. To fix it, zypper
> > +# cache should be refreshed.
> > +sudo zypper cc
> > +sudo zypper ref
> 
> Nit: It is better to use non-abbreviated commands in scripts for
> readability: clean and refresh.
> 

Thought that if the commands are clear and no need to point view
on it than it should be abbreviated. Anyway changed it.

> >  %build
> > +# openSuSE sets its own build directory in its macros, but we
> > +# want to use in-source build there to simplify the RPM spec.
> > +%if (0%{?sle_version} >= 1500)
> > +%global __builddir .
> > +%endif
> 
> It is better to move all globals upward (see where other ones are
> defined) to highlight the fact that they are rpm-spec wide, not
> section-wide.
> 

Thought that upper globals were under "if" and better not to mess
with it. Also the part of upper code is not build part. Anyway changed.

> >  # RHBZ #1301720: SYSCONFDIR an LOCALSTATEDIR must be specified explicitly
> >  %cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo \
> >           -DCMAKE_INSTALL_LOCALSTATEDIR:PATH=%{_localstatedir} \
> >           -DCMAKE_INSTALL_SYSCONFDIR:PATH=%{_sysconfdir} \
> > -%if %{with backtrace}
> > +%if (%{with backtrace} || 0%{?sle_version} >= 1500)
> >           -DENABLE_BACKTRACE:BOOL=ON \
> >  %else
> >           -DENABLE_BACKTRACE:BOOL=OFF \
> 
> We have `%bcond_without backtrace` on all distros except CentOS 8. It
> means that default %{with backtrace} is true on openSUSE. So this change
> is not needed.
> 

Ok, sure.

> > diff --git a/test/box-tap/cfg.skipcond b/test/box-tap/cfg.skipcond
> > index 33cafc12b..5f4256b6a 100644
> > --- a/test/box-tap/cfg.skipcond
> > +++ b/test/box-tap/cfg.skipcond
> > @@ -1,3 +1,4 @@
> > +import os
> >  import platform
> >  
> >  # Disabled on FreeBSD due to fail #4271:
> > @@ -5,6 +6,15 @@ import platform
> >  if platform.system() == 'FreeBSD':
> >      self.skip = 1
> >  
> > +# Disabled on openSuSE due to issue #4594:
> > +#   Test not ready to be used, because of problems
> > +#   around forking processes from Tarantool.
> > +try:
> > +    if os.popen('lsb_release -i').read().split(':')[1].strip() == 'openSUSE':
> > +        self.skip = 1
> > +except IndexError:
> > +    pass
> > +
> 
> In [1] I described the problem on FreeBSD. You should investigate the
> problem youself or ask someone, not just copy FreeBSD problem
> description.
> 
> And, please, read review comments carefully: it takes really significant
> time to dive into a new topic and give meaningful comments. And it is
> sad, when they are unintentionally discarded.
> 
> I looked a bit into the problem and tarantool actually has different
> behaviour about non-empty `vinyl_dir` directory, but empty memtx/wal
> dirs on SuSE and others. On my Linux box tarantool initializes a data
> directory and then got 'invalid instance UUID' error on the vylog
> reading (the test case is about panic added in [2], but I guess
> rebootstrap support [3] changes the actual behaviour). It seems that on
> openSUSE tarantool does not read the vinyl directory in the case: I
> guess it is so, because it does not have the space from the vylog after
> box.cfg().
> 
> Nobody will look into the issue you filed (even if (s)he interesting in
> vinyl), because it is named as 'test: box-tap/cfg test fails on SuSE
> 15.x images' and does not mention vinyl_dir at all.
> 
> It is not acceptable level of investigation to file an issue. Is test
> incorrect? Is the code works differently? Is the difference occurs in
> some rare case or for almost everyone? Without additional information
> it is not possible to give a priority to the issue, then it unlikely
> will be fixed.
> 
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018223.html
> [2]: https://github.com/tarantool/tarantool/commit/4d796a8c3e5b56fda8a8345e4c48c5cde270bcbc
> [3]: https://github.com/tarantool/tarantool/commit/06658416ab6c5ffab3fd3466315adcc54b1ef314
>

Ok, I'll provide more information on it.

> > diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt
> > index 06bfbbe9d..6191001de 100644
> > --- a/test/box/CMakeLists.txt
> > +++ b/test/box/CMakeLists.txt
> > @@ -1,3 +1,4 @@
> > +string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
> 
> There is no comment. I already asked for this in the previous review
> comments, but I'll add here: yep, I meant a comment in the code, not
> commit message.
> 
> The criteria here is simple. I just imagine that I'm a reader, which
> does not know anything we discussing now. Which questions the line may
> arise? For me they are the following:
> 
> - What the flag means?
> - Whether it is correct to allow undefined symbols?
> 
> The wording I proposed (cited below) answers to those questions, I
> guess.
> 
>  | 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.
> 

Thought that 4 places is not good decission to comment with common
message, thought that blame will be much better to investigate the
change. Anyway added.

> > diff --git a/third_party/luajit b/third_party/luajit
> > index 377198b88..818703399 160000
> > --- a/third_party/luajit
> > +++ b/third_party/luajit
> > @@ -1 +1 @@
> > -Subproject commit 377198b88382960a2f0af9c09014284e34630513
> > +Subproject commit 81870339991bd3f54fc532b631f48d8bf4aa2b57
> 
> We usually update submodules separately from tarantool changes.

Right, I thought the same, but from the other hand thought that
if not disscussed than already correct. Changed it.