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

[Alexander Turenko <alexander.turenko@tarantool.org>]

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

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

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

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

> 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

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

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