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

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

[Alexander V. Tikhonov]

Implemented openSuSE packages build with testing for images:
opensuse-leap:15.[0-2]

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

Found that openSuSE adding linker flag '--no-undefined' which produces
the fails on building tests. Decided to block this flag due to 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.

Test box-tap/cfg fails and temporary blocked according to the
issue created for it #4594: test not ready to be used, because
of problems around forking processes from Tarantool.

Added rpm/prebuild-opensuse-leap.sh script to fix the issue with
broken repositories in zypper cache. It refreshes zypper cache.

Added opensuse-leap of 15.1 and 15.2 versions to Gitlab-CI packages
building/deploing jobs.

Close #4562
---

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

 .gitlab-ci.yml                | 24 ++++++++++++++++++++++++
 rpm/prebuild-opensuse-leap.sh |  7 +++++++
 rpm/tarantool.spec            | 17 +++++++++++------
 test/app-tap/CMakeLists.txt   |  1 +
 test/app/CMakeLists.txt       |  1 +
 test/box-tap/cfg.skipcond     | 10 ++++++++++
 test/box/CMakeLists.txt       |  1 +
 third_party/luajit            |  2 +-
 tools/update_repo.sh          |  6 ++++--
 9 files changed, 60 insertions(+), 9 deletions(-)
 create mode 100755 rpm/prebuild-opensuse-leap.sh

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 3978bff56..63976721f 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -400,6 +400,18 @@ debian_10:
     OS: 'debian'
     DIST: 'buster'
 
+opensuse_15_1:
+  <<: *pack_definition
+  variables:
+    OS: 'opensuse-leap'
+    DIST: '15.1'
+
+opensuse_15_2:
+  <<: *pack_definition
+  variables:
+    OS: 'opensuse-leap'
+    DIST: '15.2'
+
 # Deploy
 
 sources_deploy:
@@ -497,6 +509,18 @@ debian_10_deploy:
     OS: 'debian'
     DIST: 'buster'
 
+opensuse_15_1_deploy:
+  <<: *deploy_definition
+  variables:
+    OS: 'opensuse-leap'
+    DIST: '15.1'
+
+opensuse_15_2_deploy:
+  <<: *deploy_definition
+  variables:
+    OS: 'opensuse-leap'
+    DIST: '15.2'
+
 # Static builds
 
 static_build:
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
+
+# 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
diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
index 88b1d6b5c..fe2e617a5 100644
--- a/rpm/tarantool.spec
+++ b/rpm/tarantool.spec
@@ -1,5 +1,5 @@
 # Enable systemd for on RHEL >= 7 and Fedora >= 15
-%if (0%{?fedora} >= 15 || 0%{?rhel} >= 7)
+%if (0%{?fedora} >= 15 || 0%{?rhel} >= 7 || 0%{?sle_version} >= 1500)
 %bcond_without systemd
 %else
 %bcond_with systemd
@@ -7,7 +7,7 @@
 
 BuildRequires: cmake >= 2.8
 BuildRequires: make
-%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7)
+%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7 || 0%{?sle_version} >= 1500)
 # RHEL 6 requires devtoolset
 BuildRequires: gcc >= 4.5
 BuildRequires: gcc-c++ >= 4.5
@@ -78,7 +78,7 @@ BuildRequires: python2-six >= 1.9.0
 BuildRequires: python2-gevent >= 1.0
 BuildRequires: python2-yaml >= 3.0.9
 %else
-%if (0%{?rhel} != 6)
+%if (0%{?rhel} != 6 || 0%{?sle_version} >= 1500)
 BuildRequires: python >= 2.7
 BuildRequires: python-six >= 1.9.0
 BuildRequires: python-gevent >= 1.0
@@ -105,7 +105,7 @@ Requires: /etc/services
 # Deps for built-in package manager
 # https://github.com/tarantool/tarantool/issues/2612
 Requires: openssl
-%if (0%{?fedora} >= 22 || 0%{?rhel} >= 8)
+%if (0%{?fedora} >= 22 || 0%{?rhel} >= 8 || 0%{?sle_version} >= 1500)
 # RHEL <= 7 doesn't support Recommends:
 Recommends: tarantool-devel
 Recommends: git-core
@@ -140,11 +140,16 @@ C and Lua/C modules.
 %setup -q -n %{name}-%{version}
 
 %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
 # 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 \
@@ -164,7 +169,7 @@ rm -rf %{buildroot}%{_datarootdir}/doc/tarantool/
 
 %check
 %if "%{_ci}" == "travis"
-%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7)
+%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7 || 0%{?sle_version} >= 1500)
 cd test && ./test-run.py --force -j 1 unit/ app/ app-tap/ box/ box-tap/ engine/ vinyl/
 %endif
 %else
diff --git a/test/app-tap/CMakeLists.txt b/test/app-tap/CMakeLists.txt
index ee67cf533..0c3c19eb6 100644
--- a/test/app-tap/CMakeLists.txt
+++ b/test/app-tap/CMakeLists.txt
@@ -1 +1,2 @@
+string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
 build_module(module_api module_api.c)
diff --git a/test/app/CMakeLists.txt b/test/app/CMakeLists.txt
index 059ee8f3d..3642ea290 100644
--- a/test/app/CMakeLists.txt
+++ b/test/app/CMakeLists.txt
@@ -1 +1,2 @@
+string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
 build_module(loaderslib loaderslib.c)
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
+
 # Disabled on OpenBSD due to fail #XXXX:
 if platform.system() == 'OpenBSD':
     self.skip = 1
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}")
 include_directories(${MSGPUCK_INCLUDE_DIRS})
 build_module(function1 function1.c)
 build_module(reload1 reload1.c)
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
diff --git a/tools/update_repo.sh b/tools/update_repo.sh
index 47eadd189..65bafe757 100755
--- a/tools/update_repo.sh
+++ b/tools/update_repo.sh
@@ -6,7 +6,7 @@ rm_dir='rm -rf'
 mk_dir='mkdir -p'
 ws_prefix=/tmp/tarantool_repo_s3
 
-alloss='ubuntu debian el fedora'
+alloss='ubuntu debian el fedora opensuse-leap'
 product=tarantool
 remove=
 force=
@@ -31,6 +31,8 @@ function get_os_dists {
         alldists='6 7 8'
     elif [ "$os" == "fedora" ]; then
         alldists='27 28 29 30 31'
+    elif [ "$os" == "opensuse-leap" ]; then
+        alldists='15.0 15.1 15.2'
     fi
 
     echo "$alldists"
@@ -934,7 +936,7 @@ if [ "$os" == "ubuntu" -o "$os" == "debian" ]; then
     # unlock the publishing
     $rm_file $ws_lockfile
     popd
-elif [ "$os" == "el" -o "$os" == "fedora" ]; then
+elif [ "$os" == "el" -o "$os" == "fedora" -o "$os" == "opensuse-leap" ]; then
     # RPM packages structure needs different paths for binaries and sources
     # packages, in this way it is needed to call the packages registering
     # script twice with the given format:
-- 
2.17.1

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

[Alexander Turenko]

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

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

[Alexander V. Tikhonov]

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.

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

[Alexander Turenko]

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

We discussed it with Alexander and decided to move this workaround right
to packpack, because it is relevant for all packpack users, not just
tarantool.