From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B0B05445320 for ; Mon, 6 Jul 2020 21:36:55 +0300 (MSK) Date: Mon, 6 Jul 2020 21:36:07 +0300 From: Alexander Turenko Message-ID: <20200706183607.zmpyiaobpiv732ov@tkn_work_nb> References: <5ac79b3ebd520ae5ffc2de998793ea3949e0d4ba.1594017314.git.avtikhon@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5ac79b3ebd520ae5ffc2de998793ea3949e0d4ba.1594017314.git.avtikhon@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2] gitlab-ci: add openSuSE packages build jobs List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Alexander V. Tikhonov" Cc: tarantool-patches@dev.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.