[Tarantool-patches] [PATCH v2] gitlab-ci: add openSuSE packages build jobs
Alexander V. Tikhonov
avtikhon at tarantool.org
Mon Jul 6 22:25:21 MSK 2020
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.
More information about the Tarantool-patches
mailing list