From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 DC47D445320 for ; Mon, 6 Jul 2020 22:25:23 +0300 (MSK) Date: Mon, 6 Jul 2020 22:25:21 +0300 From: "Alexander V. Tikhonov" Message-ID: <20200706192521.GA5425@hpalx> References: <5ac79b3ebd520ae5ffc2de998793ea3949e0d4ba.1594017314.git.avtikhon@tarantool.org> <20200706183607.zmpyiaobpiv732ov@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200706183607.zmpyiaobpiv732ov@tkn_work_nb> 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 Turenko Cc: tarantool-patches@dev.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.